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@xxxxxxxxx>
Looks good to me. You can add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
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
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
