linux-kernel - Re: do_change_type(): refuse to operate on unmounted/not ours mounts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE1zp77jmFD=rySJVLf6yU+JKZnUpjkBagC3qQHrxPotrccEbQ@mail.gmail.com>
Date: Thu, 14 Aug 2025 12:08:49 +0800
From: Pavel Tikhomirov <snorcht@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Tycho Andersen <tycho@...ho.pizza>, Andrei Vagin <avagin@...gle.com>, 
	Andrei Vagin <avagin@...il.com>, Christian Brauner <brauner@...nel.org>, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	criu@...ts.linux.dev, Linux API <linux-api@...r.kernel.org>, 
	stable <stable@...r.kernel.org>
Subject: Re: do_change_type(): refuse to operate on unmounted/not ours mounts

On Thu, Aug 14, 2025 at 3:41 AM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Wed, Aug 13, 2025 at 01:09:27PM -0600, Tycho Andersen wrote:
> > On Wed, Aug 13, 2025 at 07:56:01PM +0100, Al Viro wrote:
> > > @@ -3347,18 +3360,11 @@ static int do_set_group(struct path *from_path, struct path *to_path)
> > >
> > >     namespace_lock();
> > >
> > > -   err = -EINVAL;
> > > -   /* To and From must be mounted */
> > > -   if (!is_mounted(&from->mnt))
> > > -           goto out;
> > > -   if (!is_mounted(&to->mnt))
> > > -           goto out;
> > > -
> > > -   err = -EPERM;
> > > -   /* We should be allowed to modify mount namespaces of both mounts */
> > > -   if (!ns_capable(from->mnt_ns->user_ns, CAP_SYS_ADMIN))
> > > +   err = may_change_propagation(from);
> > > +   if (err)
> > >             goto out;
> > > -   if (!ns_capable(to->mnt_ns->user_ns, CAP_SYS_ADMIN))
> > > +   err = may_change_propagation(from);
> >
> > Just driving by, but I guess you mean "to" here.
>
> D'oh...  Yes, of course.  Fun question: would our selftests have caught
> that?
> [checks]
> move_mount_set_group_test.c doesn't have anything in that area, nothing in
> LTP or xfstests either, AFAICS...

Yes, selftest is very simple and is not covering userns checks.

>  And I don't see anything in
> https://github.com/checkpoint-restore/criu
> either - there are uses of MOVE_MOUNT_SET_GROUP, but they are well-buried
> and I don't see anything in their tests that would even try to poke into
> that thing...
>
> Before we go and try to cobble something up, does anybody know of a place
> where regression tests for MOVE_MOUNT_SET_GROUP could be picked from?
>

Basically each CRIU test that is run by zdtm (if it is in ns/uns
flavor (which are most of them)), tests mounts checkpoint/restore. And
each test which has shared/slave moutns leads to MOVE_MOUNT_SET_GROUP
being used and thus tested. We have a mountinfo comparison in zdtm
which checks that propagation is topologically the same after c/r.

But, yes, we do not cover userns checks, as in CRIU case, CRIU is
expected to run in userns which has all capabilities over restored
container, and should always pass those checks.

JFYI:

The use of MOVE_MOUNT_SET_GROUP in CRIU is well-buried in:

https://github.com/checkpoint-restore/criu/blob/116e56ba46382c05066d33a8bbadcc495dbdb644/criu/mount-v2.c#L896

  +-< move_mount_set_group
    +-< restore_one_sharing
      +-< restore_one_sharing_group
        +-< restore_mount_sharing_options
          +-< prepare_mnt_ns_v2

This stack already has a set of precreated mounts and walks over their
sharing groups saved in CRIU image files and assigns them accordingly.

And we have a bunch of tests with different sharing configurations to
test propagation c/r specifically:

git grep -l "SHARING\|SLAVE" test/zdtm/static
test/zdtm/static/mnt_ext_auto.c
test/zdtm/static/mnt_ext_master.c
test/zdtm/static/mnt_ext_multiple.c
test/zdtm/static/mnt_root_ext.c
test/zdtm/static/mntns_overmount.c
test/zdtm/static/mntns_shared_bind03.c
test/zdtm/static/mount_complex_sharing.c
test/zdtm/static/mountpoints.c
test/zdtm/static/shared_slave_mount_children.c

It should be enough to run a zdtm test-suit to check that change does
not break something for CRIU (will do).