linux-kernel - Re: [PATCH v5] statmount: accept fd as a parameter

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] [day] [month] [year] [list]

Message-ID: <CAJfpeguX8syNaGZahMeOD8cE4ZaQEropPTBMyCXLvgkHgGVjqg@mail.gmail.com>
Date: Thu, 13 Nov 2025 13:57:44 +0100
From: Miklos Szeredi <miklos@...redi.hu>
To: Bhavik Sachdev <b.sachdev1904@...il.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	criu@...ts.linux.dev, Aleksa Sarai <cyphar@...har.com>, 
	Pavel Tikhomirov <ptikhomirov@...tuozzo.com>, Jan Kara <jack@...e.cz>, 
	John Garry <john.g.garry@...cle.com>, Arnaldo Carvalho de Melo <acme@...hat.com>, 
	"Darrick J . Wong" <djwong@...nel.org>, Namhyung Kim <namhyung@...nel.org>, Ingo Molnar <mingo@...nel.org>, 
	Andrei Vagin <avagin@...il.com>, Alexander Mikhalitsyn <alexander@...alicyn.com>
Subject: Re: [PATCH v5] statmount: accept fd as a parameter

On Sun, 9 Nov 2025 at 06:40, Bhavik Sachdev <b.sachdev1904@...il.com> wrote:

> diff --git a/fs/namespace.c b/fs/namespace.c
> index d82910f33dc4..153c0ea85386 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -5207,6 +5207,12 @@ static int statmount_mnt_root(struct kstatmount *s, struct seq_file *seq)
>         return 0;
>  }
>
> +static int statmount_mnt_point_unmounted(struct kstatmount *s, struct seq_file *seq)
> +{
> +       seq_puts(seq, "[unmounted]");

Please no, There's statmount.mask for this purpose exactly.

>  static int statmount_mnt_point(struct kstatmount *s, struct seq_file *seq)
>  {
>         struct vfsmount *mnt = s->mnt;
> @@ -5262,7 +5268,10 @@ static int statmount_sb_source(struct kstatmount *s, struct seq_file *seq)
>  static void statmount_mnt_ns_id(struct kstatmount *s, struct mnt_namespace *ns)
>  {
>         s->sm.mask |= STATMOUNT_MNT_NS_ID;
> -       s->sm.mnt_ns_id = ns->ns.ns_id;
> +       if (ns)
> +               s->sm.mnt_ns_id = ns->ns.ns_id;
> +       else
> +               s->sm.mnt_ns_id = 0;

Same here.  Please clear mask if the requested field cannot be meaningfully set.

>  /* locks: namespace_shared */
>  static int do_statmount(struct kstatmount *s, u64 mnt_id, u64 mnt_ns_id,
> -                       struct mnt_namespace *ns)
> +                       struct mnt_namespace *ns, unsigned int flags)
>  {
>         struct mount *m;
>         int err;
>
>         /* Has the namespace already been emptied? */
> -       if (mnt_ns_id && mnt_ns_empty(ns))
> +       if (!(flags & STATMOUNT_BY_FD) && mnt_ns_id && mnt_ns_empty(ns))
>                 return -ENOENT;

This check does not make sense without the lookup_mnt_in_ns(), so
please move it right before the lookup.

>
> -       s->mnt = lookup_mnt_in_ns(mnt_id, ns);
> -       if (!s->mnt)
> -               return -ENOENT;
> +       if (!(flags & STATMOUNT_BY_FD)) {
> +               s->mnt = lookup_mnt_in_ns(mnt_id, ns);

A comment would be nice about s->mnt being set in caller for the
STATMOUNT_BY_FD case.

> -       ns = grab_requested_mnt_ns(&kreq);
> -       if (!ns)
> -               return -ENOENT;
> +       if (flags & STATMOUNT_BY_FD) {
> +               file_from_fd = fget_raw(kreq.fd);
> +               if (!file_from_fd)
> +                       return -EBADF;
> +
> +               fd_mnt = file_from_fd->f_path.mnt;
> +               ns = real_mount(fd_mnt)->mnt_ns;
> +               if (ns)
> +                       refcount_inc(&ns->passive);
> +               else
> +                       if (!capable(CAP_SYS_ADMIN))
> +                               return -ENOENT;

Why is this not EPERM?

I think this check belongs together with the  ns_capable_noaudit() check below.

> +       } else {
> +               ns = grab_requested_mnt_ns(&kreq, flags);
> +               if (!ns)
> +                       return -ENOENT;
> +       }
>
> -       if (kreq.mnt_ns_id && (ns != current->nsproxy->mnt_ns) &&
> +       if (ns && (ns != current->nsproxy->mnt_ns) &&
>             !ns_capable_noaudit(ns->user_ns, CAP_SYS_ADMIN))
>                 return -ENOENT;

(This should also be EPERM?  Searate patch, though).

Something like this:

        if (ns) {
                if ((ns != current->nsproxy->mnt_ns) &&
                    !ns_capable_noaudit(ns->user_ns, CAP_SYS_ADMIN))
                        return -ENOENT;
        } else if (!capable(CAP_SYS_ADMIN))
                return -EPERM;
        }

Thanks,
Miklos