linux-kernel - Re: [PATCH 16/27] r-o-bind-mounts-elevate-write-count-for-some-ioctls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20071106090102.GA32704@atrey.karlin.mff.cuni.cz>
Date:	Tue, 6 Nov 2007 10:01:02 +0100
From:	Jan Kara <jack@...e.cz>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Dave Hansen <haveblue@...ibm.com>, linux-kernel@...r.kernel.org,
	miklos@...redi.hu, hch@...radead.org, Jan Kara <jack@....cz>
Subject: Re: [PATCH 16/27] r-o-bind-mounts-elevate-write-count-for-some-ioctls

> On Thu, 01 Nov 2007 16:08:47 -0700
> Dave Hansen <haveblue@...ibm.com> wrote:
> 
> > Some ioctl()s can cause writes to the filesystem.  Take these, and make them
> > use mnt_want/drop_write() instead.
> > 
> > We need to pass the filp one layer deeper in XFS, but somebody _just_ pulled
> > it out in February because nobody was using it, so I don't feel guilty for
> > adding it back.
> 
> See, when we combine this patch with Jan's
> forbid-user-to-change-file-flags-on-quota-files.patch we silently add bugs
> to five filesystems.  Lessons:
> 
> - never ever ever do `return' from deep in the guts of a function.  This
>   is a *classic* instance of the maintainability risks which this practice
>   introduces.
  OK, lesson learned ;) Thanks for spotting this. But if there already
was a label like:

err_out:
   return err;

  And I would have just added a place which does 'goto err_out', then
the same problem could arise... But I agree it's less likely than if we
do just return err;.

> - this whole elevate-the-write-count-in-a-zillion-places stuff is quite
>   fragile.
> 
> diff -puN fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files fs/ext2/ioctl.c
> --- a/fs/ext2/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/ext2/ioctl.c
> @@ -57,7 +57,8 @@ int ext2_ioctl (struct inode * inode, st
>  		/* Is it quota file? Do not allow user to mess with it */
>  		if (IS_NOQUOTA(inode)) {
>  			mutex_unlock(&inode->i_mutex);
> -			return -EPERM;
> +			ret = -EPERM;
> +			goto setflags_out;
>  		}
>  		oldflags = ei->i_flags;
>  
> diff -puN fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files fs/ext3/ioctl.c
> --- a/fs/ext3/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/ext3/ioctl.c
> @@ -60,7 +60,8 @@ int ext3_ioctl (struct inode * inode, st
>  		/* Is it quota file? Do not allow user to mess with it */
>  		if (IS_NOQUOTA(inode)) {
>  			mutex_unlock(&inode->i_mutex);
> -			return -EPERM;
> +			err = -EPERM;
> +			goto flags_out;
>  		}
>  		oldflags = ei->i_flags;
>  
> diff -puN fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files fs/ext4/ioctl.c
> --- a/fs/ext4/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/ext4/ioctl.c
> @@ -59,7 +59,8 @@ int ext4_ioctl (struct inode * inode, st
>  		/* Is it quota file? Do not allow user to mess with it */
>  		if (IS_NOQUOTA(inode)) {
>  			mutex_unlock(&inode->i_mutex);
> -			return -EPERM;
> +			err = -EPERM;
> +			goto flags_out;
>  		}
>  		oldflags = ei->i_flags;
>  
> diff -puN fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files fs/jfs/ioctl.c
> --- a/fs/jfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files
> +++ a/fs/jfs/ioctl.c
> @@ -85,8 +85,10 @@ int jfs_ioctl(struct inode * inode, stru
>  			flags &= ~JFS_DIRSYNC_FL;
>  
>  		/* Is it quota file? Do not allow user to mess with it */
> -		if (IS_NOQUOTA(inode))
> -			return -EPERM;
> +		if (IS_NOQUOTA(inode)) {
> +			err = -EPERM;
> +			goto setflags_out;
> +		}
>  		jfs_get_inode_flags(jfs_inode);
>  		oldflags = jfs_inode->mode2;
>  
> diff -puN fs/reiserfs/ioctl.c~r-o-bind-mounts-elevate-write-count-for-some-ioctls-vs-forbid-user-to-change-file-flags-on-quota-files fs/reiserfs/ioctl.c
> _
  Why there's no modification for reiserfs?

								Honza
-- 
Jan Kara <jack@...e.cz>
SuSE CR Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/