From: Jan Kara Subject: Re: [PATCH 2/5] ext2: preserve i_mode if ext2_set_acl() fails Date: Mon, 17 Jul 2017 18:33:04 +0200 Message-ID: <20170717163304.GB22195@quack2.suse.cz> References: <099e3ef2bf3a9ab2a1d545b65ba15fad359f2c03.1499805487.git.ernesto.mnd.fernandez@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Jan Kara , Theodore Ts'o , Andreas Dilger , Dave Kleikamp , linux-ext4@vger.kernel.org, jfs-discussion@lists.sourceforge.net, reiserfs-devel@vger.kernel.org To: Ernesto =?iso-8859-1?Q?A=2E_Fern=E1ndez?= Return-path: Received: from mx2.suse.de ([195.135.220.15]:52691 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751291AbdGQQdH (ORCPT ); Mon, 17 Jul 2017 12:33:07 -0400 Content-Disposition: inline In-Reply-To: <099e3ef2bf3a9ab2a1d545b65ba15fad359f2c03.1499805487.git.ernesto.mnd.fernandez@gmail.com> Sender: linux-ext4-owner@vger.kernel.org List-ID: On Wed 12-07-17 06:54:19, Ernesto A. Fern?ndez wrote: > When changing a file's acl mask, ext2_set_acl() will first set the group > bits of i_mode to the value of the mask, and only then set the actual > extended attribute representing the new acl. > > If the second part fails (due to lack of space, for example) and the file > had no acl attribute to begin with, the system will from now on assume > that the mask permission bits are actual group permission bits, potentially > granting access to the wrong users. > > Prevent this by only changing the inode mode after the acl has been set. > > Signed-off-by: Ernesto A. Fern?ndez Thanks for the patch! I have rebased it on top of the fix in that area that is sitting in my tree and merged it to my tree. Honza > --- > A line in acl.c is too long, and checkpatch.pl complains about it when run > against this patch. In case it matters, the next patch will fix that. > > fs/ext2/acl.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/fs/ext2/acl.c b/fs/ext2/acl.c > index 79dafa7..4e04b7e 100644 > --- a/fs/ext2/acl.c > +++ b/fs/ext2/acl.c > @@ -185,16 +185,17 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type) > void *value = NULL; > size_t size = 0; > int error; > + int update_mode = 0; > + umode_t mode = inode->i_mode; > > switch(type) { > case ACL_TYPE_ACCESS: > name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS; > if (acl) { > - error = posix_acl_update_mode(inode, &inode->i_mode, &acl); > + error = posix_acl_update_mode(inode, &mode, &acl); > if (error) > return error; > - inode->i_ctime = current_time(inode); > - mark_inode_dirty(inode); > + update_mode = 1; > } > break; > > @@ -216,8 +217,14 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type) > error = ext2_xattr_set(inode, name_index, "", value, size, 0); > > kfree(value); > - if (!error) > + if (!error) { > set_cached_acl(inode, type, acl); > + if (update_mode) { > + inode->i_mode = mode; > + inode->i_ctime = current_time(inode); > + mark_inode_dirty(inode); > + } > + } > return error; > } > > -- > 2.1.4 > > -- Jan Kara SUSE Labs, CR