linux-kernel - Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

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: <CAHk-=wgEjs8bwSMSpoyFRiUT=_NEFzF8BXFEvYzVQCu8RD=WmA@mail.gmail.com>
Date: Tue, 26 Nov 2024 12:11:06 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Kees Cook <kees@...nel.org>
Cc: linux-kernel@...r.kernel.org, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christophe JAILLET <christophe.jaillet@...adoo.fr>, "Eric W. Biederman" <ebiederm@...ssion.com>, 
	Nir Lichtman <nir@...htman.org>, Tycho Andersen <tandersen@...flix.com>, 
	Vegard Nossum <vegard.nossum@...cle.com>
Subject: Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

On Mon, 25 Nov 2024 at 21:10, Kees Cook <kees@...nel.org> wrote:
>
> For the new implementation, do you want to wait a full dev cycle for
> it to bake in -next or should I send what I proposed based on your and
> Al's suggestions for this merge window?

So honestly, the more I look at our current implementation, the more I
dislike this code.

And it looks like __set_task_comm() is actually buggy right now,
because while we have a comment in linux/sched.h that says

 *   The strscpy_pad() in __set_task_comm() can ensure that the task comm is
 *   always NUL-terminated and zero-padded.

that isn't actually true, because it looks like sized_strscpy()
actually adds the final NUL at the end. I think that's because Andrew
only merged a partial patch series.

The task_lock() doesn't help that issue, because readers don't take it
(and never really did: the '%s'+tsk->comm pattern has always existed).

End result: I think we should get rid of the pointless task_lock,
explicitly make sure it's NUL-terminated, and do the actual comm[]
setup in alloc_bprm() where we make all these decisions anyway.

So something like the attached. But it's *ENTIRELY* untested.  It
looks ObviouslyCorrect(tm), but hey, things always do until somebody
finds some obvious bug. If this tests ok, I think it could make 6.13,
but ...

And I'm looking at the other uses of bprm->filename for the fdpath
case, and it's all horrible. Yes, it's the scripting, but it's also
bprm->exec + AT_EXECFN. So we're passing in those fake pathnames that
won't actually work if the fd is used for something else, and that I
think could be used as an attack vector if any user space actually
trusts them.

That all looks horrid. I *really* wish we generated the real pathname instead.

Oh well. That's a separate issue.

      Linus