On Tue, Jun 01, 2010 at 01:39:37PM +0200, Christoph Hellwig wrote:
> Make sure we check the truncate constraints early on in ->setattr by adding
> those checks to inode_change_ok. Also clean up and document inode_change_ok
> to make this obvious.
>
> As a fallout we don't have to call inode_newsize_ok from simple_setsize and
> simplify it down to a truncate_setsize which doesn't return an error. This
> simplifies a lot of setattr implementations and means we use truncate_setsize
> almost everywhere. Get rid of fat_setsize now that it's trivial and mark
> ext2_setsize static to make the calling convention obvious.
>
> Keep the inode_newsize_ok in vmtruncate for now as all callers need an
> audit for it's removal anyway.
>
> Note: setattr code in ecryptfs doesn't call inode_change_ok at all and
> needs a deeper audit, but that is left for later.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
>
> Index: linux-2.6/fs/attr.c
> ===================================================================
> --- linux-2.6.orig/fs/attr.c 2010-06-01 12:19:47.987277079 +0200
> +++ linux-2.6/fs/attr.c 2010-06-01 12:48:26.063005672 +0200
> @@ -14,35 +14,53 @@
> #include <linux/fcntl.h>
> #include <linux/security.h>
>
> -/* Taken over from the old code... */
> -
> -/* POSIX UID/GID verification for setting inode attributes. */
> +/**
> + * inode_change_ok - check if attribute changes to an inode are allowed
> + * @inode: inode to check
> + * @attr: attributes to change
> + *
> + * Check if we are allowed to change the attributes contained in @attr
> + * in the given inode. This includes the normal unix access permission
> + * checks, as well as checks for rlimits and others.
> + *
> + * Should be called as the first thing in ->setattr implementations,
> + * possibly after taking additional locks.
> + */
> int inode_change_ok(const struct inode *inode, struct iattr *attr)
> {
> - int retval = -EPERM;
> unsigned int ia_valid = attr->ia_valid;
>
> + /*
> + * First check size constraints. These can't be overriden using
> + * ATTR_FORCE.
> + */
> + if (attr->ia_mode & ATTR_SIZE) {
> + int error = inode_newsize_ok(inode, attr->ia_size);
> + if (error)
> + return error;
> + }
Hmm, I don't know if we can do this unless you have audited the
filesystems (in which case they should be on the cc list of this
pach).
The problem is whether the i_size is valid and stable at this
point. And it doesn't help even if you do leave the inode_newsize_ok
check inside the vmtruncate part of the fs if the check incorrectly
fails here.
ocfs2 performs inode_change_ok outside ocfs2_rw_lock and
ocfs2_inode_lock, and inode_newsize_ok inside; cifs holds i_lock
while checking inode_newsize_ok and updating size; gfs2 inside
gfs2_trans_begin.
Haven't looked closely at all fses or the consequences of the above.
Thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
