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);
