xfs
[Top] [All Lists]

Re: [PATCH 2/4] xfs: reject completely bogus remount options

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH 2/4] xfs: reject completely bogus remount options
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 14 Oct 2013 08:52:08 +1100
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52584D56.7090902@xxxxxxxxxxx>
References: <52584C8A.1060808@xxxxxxxxxx> <52584D56.7090902@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Fri, Oct 11, 2013 at 02:11:18PM -0500, Eric Sandeen wrote:
> There's a long comment about handling non-remountable
> options in xfs_fs_remount, but nothing addresses the case
> of completely bogus mount options at remount time, which
> can lead to some severe strangeness:
> 
> # for I in `seq 1 10`; do mount -o remount,noacl /mnt/test2; done
> # for I in `seq 1 10`; do mount -o remount,badoption /mnt/test2; done
> # grep sdb4 /etc/mtab
> /dev/sdb4 /mnt/test2 xfs 
> rw,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,noacl,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption,badoption
>  0 0
> 
> This is a bit of a hack, but we can re-use xfs_parseargs()
> with a dummy mount struct to just vet all of the remount
> options which were passed in.  With this, we get a saner
> result:
> 
> [44898.102990] EXT4-fs (sdb4): Unrecognized mount option "badoption" or 
> missing value

ext4? Really? :)

> +++ b/fs/xfs/xfs_super.c
> @@ -1202,11 +1202,25 @@ xfs_fs_remount(
>       int                     *flags,
>       char                    *options)
>  {
> -     struct xfs_mount        *mp = XFS_M(sb);
> +     struct xfs_mount        *mp = XFS_M(sb), *dummy_mp;
>       substring_t             args[MAX_OPT_ARGS];
>       char                    *p;
>       int                     error;
>  
> +     /*
> +      * Check all the mount options presented to be sure
> +      * there's nothing too crazy in there.  Non-remountable
> +      * but valid options are a different issue.
> +      */
> +     dummy_mp = kmem_zalloc(sizeof(*dummy_mp), KM_MAYFAIL);
> +     if (dummy_mp) {
> +             dummy_mp->m_super = sb;
> +             error = xfs_parseargs(dummy_mp, options);
> +             kfree(dummy_mp);
> +             if (error)
> +                     return -error;

This, at minimum, leaks dummy_mp->m_fsname, and it will leak other
strings that are also kstrdup()d by xfs_parseargs().

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>