linux-kernel - Re: [PATCH] pid: Add a judgment for ns null in pid_nr

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: <CAGudoHHY7dMmxAc7x0avSxpNz-MfitQa-Shv2MSisLm-r4GH-A@mail.gmail.com>
Date: Mon, 1 Sep 2025 17:55:44 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Christian Brauner <brauner@...nel.org>, Xiang Gao <gxxa03070307@...il.com>, joel.granados@...nel.org, 
	lorenzo.stoakes@...cle.com, linux-kernel@...r.kernel.org, 
	gaoxiang17 <gaoxiang17@...omi.com>, Liam.Howlett@...cle.com, viro@...iv.linux.org.uk
Subject: Re: [PATCH] pid: Add a judgment for ns null in pid_nr_ns

On Mon, Sep 1, 2025 at 5:44 PM Mateusz Guzik <mjguzik@...il.com> wrote:
>
> On Mon, Sep 1, 2025 at 5:32 PM Oleg Nesterov <oleg@...hat.com> wrote:
> >
> > ping...
> >
> > We need either
> >
> >   [1/1] pid: Add a judgment for ns null in pid_nr_ns
> >   https://git.kernel.org/vfs/vfs/c/006568ab4c5c
> >
> > or
> >
> >   [1/4] pid: make __task_pid_nr_ns(ns => NULL) safe for zombie callers
> >   https://git.kernel.org/vfs/vfs/c/abdfd4948e45
> >
> > in any case imo the changelog should explain why do we care
> > to check ns != NUll, "Sometimes null is returned for task_active_pid_ns"
> > doesn't look like a good explanation...
> >
>
> Since I caught this a stray patchset I'll bite: given the totally
> arbitrary task struct in an irq handler, why even allow querying it
> from that level? The task is literally random, and even possibly dead
> as in this crash report.
>
> To my reading the code which runs into woes here is private to a
> vendor. Maybe I missed something, but I don't see a justification for
> querying the task in an irq handler to begin with (and per above I
> don't understand what the point is).
>
> That is to say, if this was up to me, I would at best assert we are in
> the process context and that ns is not NULL. As a result I would very
> much *ban* the call as reported here, unless there is a good reason to
> make it work (what is it?).
>
> That's my side rant, feel free to ignore. :->
>

Maybe even go a little further and assert that the task at hand is
fully constructed and not exiting yet.

The worry here is that normally you would expect the task to be all
there (so to speak).

For example imagine the task is mid-teardown, with some pointers
already freed and whatnot and interrupt comes in. Code might show up
which will deref parts of task_struct which are no longer legally
accessible, but this only blows up if you are unlucky enough while
racing. In practice these accesses might even never trip over anything
despite being illegal (unless someone nullifies a pointer or
something), hiding the problem.

All in all, this is a can of worms and from my cursory reading no
justification for doing allowing it was provided.
-- 
Mateusz Guzik <mjguzik gmail.com>