| Message ID | 4d0758dad0ee32bda49113a957a8b5d9ce882e9b.1501124092.git.ernesto.mnd.fernandez@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu 27-07-17 00:10:20, Ernesto A. Fernández wrote: > When changing a file's acl mask, hfsplus_set_posix_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 <ernesto.mnd.fernandez@gmail.com> Looks good to me. You can add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > The same issue affects several filesystems; some of them have already > applied patches, see for example: > > fe26569 ext2: preserve i_mode if ext2_set_acl() fails > > In order to test this I had to add a mount option to enable acls. That > patch is sent next. > > fs/hfsplus/posix_acl.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c > index 6bb5d7c..24a1cdf 100644 > --- a/fs/hfsplus/posix_acl.c > +++ b/fs/hfsplus/posix_acl.c > @@ -102,13 +102,19 @@ static int __hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, > int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, int type) > { > int err; > + int update_mode = 0; > + umode_t mode = inode->i_mode; > > if (type == ACL_TYPE_ACCESS && acl) { > - err = posix_acl_update_mode(inode, &inode->i_mode, &acl); > + err = posix_acl_update_mode(inode, &mode, &acl); > if (err) > return err; > + update_mode = 1; > } > - return __hfsplus_set_posix_acl(inode, acl, type); > + err = __hfsplus_set_posix_acl(inode, acl, type); > + if (!err && update_mode) > + inode->i_mode = mode; > + return err; > } > > int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir) > -- > 2.1.4 >
diff --git a/fs/hfsplus/posix_acl.c b/fs/hfsplus/posix_acl.c index 6bb5d7c..24a1cdf 100644 --- a/fs/hfsplus/posix_acl.c +++ b/fs/hfsplus/posix_acl.c @@ -102,13 +102,19 @@ static int __hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, int hfsplus_set_posix_acl(struct inode *inode, struct posix_acl *acl, int type) { int err; + int update_mode = 0; + umode_t mode = inode->i_mode; if (type == ACL_TYPE_ACCESS && acl) { - err = posix_acl_update_mode(inode, &inode->i_mode, &acl); + err = posix_acl_update_mode(inode, &mode, &acl); if (err) return err; + update_mode = 1; } - return __hfsplus_set_posix_acl(inode, acl, type); + err = __hfsplus_set_posix_acl(inode, acl, type); + if (!err && update_mode) + inode->i_mode = mode; + return err; } int hfsplus_init_posix_acl(struct inode *inode, struct inode *dir)
When changing a file's acl mask, hfsplus_set_posix_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 <ernesto.mnd.fernandez@gmail.com> --- The same issue affects several filesystems; some of them have already applied patches, see for example: fe26569 ext2: preserve i_mode if ext2_set_acl() fails In order to test this I had to add a mount option to enable acls. That patch is sent next. fs/hfsplus/posix_acl.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)