[PATCH] coredump: Do not lock during 'comm' reporting

[PATCH] coredump: Do not lock during 'comm' reporting

[Kees Cook]

The 'comm' member will always be NUL terminated, and this is not
fast-path, so we can just perform a direct memcpy during a coredump
instead of potentially deadlocking while holding the task struct lock.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Closes: https://lore.kernel.org/all/d122ece6-3606-49de-ae4d-8da88846bef2@oracle.com
Fixes: c114e9948c2b ("coredump: Standartize and fix logging")
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Allen Pais <apais@linux.microsoft.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Roman Kisel <romank@linux.microsoft.com>
Cc: Xiaoming Ni <nixiaoming@huawei.com>

Vegard, can you validate that this fixes the problem for you? I have
been wrecked by covid, so very slow to respond here. There's a related
thread about this locked, but we can just totally bypass it here.

---
 include/linux/coredump.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/coredump.h b/include/linux/coredump.h
index edeb8532ce0f..a99079115a38 100644
--- a/include/linux/coredump.h
+++ b/include/linux/coredump.h
@@ -52,8 +52,8 @@ extern int do_coredump(const kernel_siginfo_t *siginfo);
 #define __COREDUMP_PRINTK(Level, Format, ...) \
 	do {	\
 		char comm[TASK_COMM_LEN];	\
-	\
-		get_task_comm(comm, current);	\
+		/* This will always be NUL terminated. */ \
+		memcpy(comm, current->comm, sizeof(comm)); \
 		printk_ratelimited(Level "coredump: %d(%*pE): " Format "\n",	\
 			task_tgid_vnr(current), (int)strlen(comm), comm, ##__VA_ARGS__);	\
 	} while (0)	\
-- 
2.34.1

Re: [PATCH] coredump: Do not lock during 'comm' reporting

[Andrew Morton]

On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote:

> The 'comm' member will always be NUL terminated,

Why is this?  I thought this is only true if the caller holds task_lock()?

> and this is not
> fast-path, so we can just perform a direct memcpy during a coredump
> instead of potentially deadlocking while holding the task struct lock.
> 

Re: [PATCH] coredump: Do not lock during 'comm' reporting

[Kees Cook]

On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote:
> On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote:
> 
> > The 'comm' member will always be NUL terminated,
> 
> Why is this?  I thought this is only true if the caller holds task_lock()?

Because it's always written with strscpy_pad(). The final byte will
always be NUL. (And this has been true for a very long time.)

-- 
Kees Cook

Re: [PATCH] coredump: Do not lock during 'comm' reporting

[Andrew Morton]

On Sat, 28 Sep 2024 14:39:45 -0700 Kees Cook <kees@kernel.org> wrote:

> On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote:
> > On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote:
> > 
> > > The 'comm' member will always be NUL terminated,
> > 
> > Why is this?  I thought this is only true if the caller holds task_lock()?
> 
> Because it's always written with strscpy_pad(). The final byte will
> always be NUL. (And this has been true for a very long time.)

So why does __get_task_comm() need to take task_lock()?

Re: [PATCH] coredump: Do not lock during 'comm' reporting

[Kees Cook]

On Sat, Sep 28, 2024 at 02:46:36PM -0700, Andrew Morton wrote:
> On Sat, 28 Sep 2024 14:39:45 -0700 Kees Cook <kees@kernel.org> wrote:
> 
> > On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote:
> > > On Sat, 28 Sep 2024 14:08:31 -0700 Kees Cook <kees@kernel.org> wrote:
> > > 
> > > > The 'comm' member will always be NUL terminated,
> > > 
> > > Why is this?  I thought this is only true if the caller holds task_lock()?
> > 
> > Because it's always written with strscpy_pad(). The final byte will
> > always be NUL. (And this has been true for a very long time.)
> 
> So why does __get_task_comm() need to take task_lock()?

That was to make sure we didn't end up with garbled results, but
discussions have determined that we don't care:
https://lore.kernel.org/all/20240828030321.20688-1-laoar.shao@gmail.com/

But just for safety's sake, I'll change this memcpy to:

    memcpy_and_pad(comm, sizeof(comm), current->comm, sizeof(comm) - 1, 0);

-- 
Kees Cook

Re: [PATCH] coredump: Do not lock during 'comm' reporting

[Vegard Nossum]

On 28/09/2024 23:51, Kees Cook wrote:
> On Sat, Sep 28, 2024 at 02:46:36PM -0700, Andrew Morton wrote:
>> On Sat, 28 Sep 2024 14:39:45 -0700 Kees Cook <kees@kernel.org> wrote:
>>> On Sat, Sep 28, 2024 at 02:35:32PM -0700, Andrew Morton wrote:
>> So why does __get_task_comm() need to take task_lock()?
> 
> That was to make sure we didn't end up with garbled results, but
> discussions have determined that we don't care:
> https://lore.kernel.org/all/20240828030321.20688-1-laoar.shao@gmail.com/
> 
> But just for safety's sake, I'll change this memcpy to:
> 
>      memcpy_and_pad(comm, sizeof(comm), current->comm, sizeof(comm) - 1, 0);
> 

Reverting Linus's revert, applying the patch in this thread, and then
changing it to that memcpy_and_pad above, everything seems to work well.

Tested-by: Vegard Nossum <vegard.nossum@oracle.com>

(However, I have not looked at the _safety_ of omitting task_lock()...)

I'm also wondering how I could successfully cat /proc/$pid/status before
on one of these hung processes given that it does __get_task_comm(),
which does task_lock(), which should have presumably hung as well?

Finally, I'll add that the comm/pid format is different from some other
messages:

kernel: coredump: 31447(entry-fuzz.1): coredump has not been created,
error -13
kernel: entry-fuzz.1[43396] bad frame in rt_sigreturn
frame:00000000e4ec200e ip:40a6712e sp:40c25f20 orax:f


Vegard