Re: [PATCH v5 2/6] selftests/x86: fix fork bug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] selftests/x86: missing Fixes trailer and redundant report text
  in changelog
--

commit 63564f14db64d744c41e3438f86b4c7188ec7e2a
Author: Bill Roberts <bill.roberts@arm.com>

> selftests/x86: fix fork bug
>
> The shadow stack ptrace test does not check whether fork() fails
> before proceeding with the parent-side ptrace operations.
>
> If fork() fails, it returns -1, which is subsequently passed to
> kill() on the test's error path. Calling kill(-1, SIGKILL) sends
> SIGKILL to every process the caller has permission to signal,
> potentially terminating unrelated processes.
>
> Check for fork() failure immediately and return an error before
> entering the ptrace test's parent-side logic.
>
> Shashiko pointed out this fork bug, and the relevant snippets from the
> report are below:
>
> [ ... ]
> > +int test_ptrace(void)
> > +{
> > +     unsigned long saved_ssp, ssp = 0;
> > +     struct sigaction sa = {};
> > +     struct iovec iov;
> > +     int status;
> > +     int pid;
> > +
> > +     iov.iov_base = &ssp;
> > +     iov.iov_len = sizeof(ssp);
> > +
> > +     pid = fork();
> > +     if (!pid) {
>
> [Severity: High]
> This is a pre-existing issue, but could this code broadcast SIGKILL to
> all user processes if fork() fails?
>
> If fork() returns -1 due to resource limits, execution will proceed with
> pid = -1. When ptrace() subsequently fails, the error path is triggered:
>
> out_kill:
>         kill(pid, SIGKILL);
>         return 1;
>
> Since pid is -1, POSIX specifies that this will kill all processes the
> current user has permission to terminate, potentially crashing the host
> environment.
>
> Signed-off-by: Bill Roberts <bill.roberts@arm.com>

[Severity: Low]
This fixes a bug in test_ptrace() introduced with the shadow stack test.
Should the commit message include:

  Fixes: 81f30337ef4f ("selftests/x86: Add shadow stack test")

Also, this isn't a bug, but the subject does not mention shadow_stack or
test_ptrace(), and the pasted review snippet in the second half of the
changelog repeats the explanation already given in the first three
paragraphs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182758.252223-1-bill.roberts@arm.com?part=2