On Mon, 25 Nov 2024 at 21:10, Kees Cook <kees@kernel.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