linux-kernel - Re: [PATCH] pidfd: prevent creation of pidfds for kthreads

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: <20240801080120.GA4038@redhat.com>
Date: Thu, 1 Aug 2024 10:01:20 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: Christian Brauner <brauner@...nel.org>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Aleksa Sarai <cyphar@...har.com>,
	Tycho Andersen <tandersen@...flix.com>,
	Daan De Meyer <daan.j.demeyer@...il.com>, Tejun Heo <tj@...nel.org>,
	stable@...r.kernel.org
Subject: Re: [PATCH] pidfd: prevent creation of pidfds for kthreads

OK, I won't argue, but ....

On 08/01, Christian Brauner wrote:
>
> On Wed, Jul 31, 2024 at 04:51:33PM GMT, Oleg Nesterov wrote:
> > On 07/31, Christian Brauner wrote:
> > >
> > > It's currently possible to create pidfds for kthreads but it is unclear
> > > what that is supposed to mean. Until we have use-cases for it and we
> > > figured out what behavior we want block the creation of pidfds for
> > > kthreads.
> >
> > Hmm... could you explain your concerns? Why do you think we should disallow
> > pidfd_open(pid-of-kthread) ?
>
> It basically just works now and it's not intentional - at least not on
> my part. You can't send signals to them,

Yes, you can't send signals to kthread. So what?

You can't send signals to the normal processes if check_kill_permission()
fails. And even if you are root, you can't send an unhandled signal via
pidfd = pidfd_open(1).

> you may or may not get notified
> via poll when a kthread exits.

Why? the exiting kthread should not differ in this respect?

> (So imho this causes more confusion then it is actually helpful. If we
> add supports for kthreads I'd also like pidfs to gain a way to identify
> them via statx() or fdinfo.)

/proc/$pid/status has a "Kthread" field...

> > > @@ -2403,6 +2416,12 @@ __latent_entropy struct task_struct *copy_process(
> > >  	if (clone_flags & CLONE_PIDFD) {
> > >  		int flags = (clone_flags & CLONE_THREAD) ? PIDFD_THREAD : 0;
> > >
> > > +		/* Don't create pidfds for kernel threads for now. */
> > > +		if (args->kthread) {
> > > +			retval = -EINVAL;
> > > +			goto bad_fork_free_pid;
> >
> > Do we really need this check? Userspace can't use args->kthread != NULL,
> > the kernel users should not use CLONE_PIDFD.
>
> Yeah, I know. That's really just proactive so that user of e.g.,
> copy_process() such as vhost or so on don't start handing out pidfds for
> stuff without requring changes to the helper itself.

Then I'd suggest WARN_ON_ONCE(args->kthread).

But as I said I won't argue. I see nothing wrong in this patch.

Oleg.