Re: [PATCH 2/3] fs: Allow user to lock mount attributes with mount_setattr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Aug 13, 2023 at 10:41 PM Aleksa Sarai <cyphar@xxxxxxxxxx> wrote:
>
> It just occurred to me that the whole MNT_LOCK_* machinery has the
> unfortunate consequence of restricting the host root user from being
> able to modify the locked flags. Since this change will let you do this
> without creating a userns, do we want to make can_change_locked_flags()
> do capable(CAP_SYS_MOUNT)?
>
Doesn't mount_setattr already require that the user has CAP_SYS_ADMIN
in the mount's user namespace?

I'm not sure how this lets us bypass that.

Or are you saying we should check for
CAP_SYS_MOUNT && CAP_SYS_ADMIN?

> > +             if ((new_mount_flags & kattr->attr_lock) != kattr->attr_lock) {
> > +                     err = -EINVAL;
> > +                     break;
> > +             }
>
> Since the MNT_LOCK_* flags are invisible to userspace, it seems more
> reasonable to have the attr_lock set be added to the existing set rather
> than requiring userspace to pass the same set of flags.
>
IMHO, it's nice to be able to use the existing set of flags. I don't mind
adding new flags though.

> Actually, AFAICS this implementation breaks backwards compatibility
> because with this change you now need to pass MNT_LOCK_* flags if
> operating on a mount that has locks applied already. So existing
> programs (which have .attr_lock=0) will start getting -EINVAL when
> operating on mounts with locked flags (such as those locked in the
> userns case). Or am I missing something?
I don't think so, because if attr_lock is 0, then
new_mount_flags & kattr->attr_lock is 0. kattr->attr_lock is only
flags to *newly lock*, and doesn't inherit the set of current locks.

>
> In any case, the most reasonable behaviour would be to OR the requested
> lock flags with the existing ones IMHO.
>
I can append this to the mount_attr_do_lock test in the next patch, and it
lets me flip the NOSUID bit on and off without attr_lock being set, unless
you're referring to something else. You shouldn't be able to attr_clr a
locked attribute (even as CAP_SYS_ADMIN in the init_user_ns).
attr_set will noop.


/* Make sure we can set NOSUID (not locked) */
memset(&attr, 0, sizeof(attr));
attr.attr_set = MOUNT_ATTR_NOSUID;
ASSERT_EQ(sys_mount_setattr(-1, "/tmp/C", 0, &attr, sizeof(attr)), 0);

/* Make sure we can clear NOSUID (not locked) */
memset(&attr, 0, sizeof(attr));
attr.attr_clr = MOUNT_ATTR_NOSUID;
ASSERT_EQ(sys_mount_setattr(-1, "/tmp/C", 0, &attr, sizeof(attr)), 0);




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux