Re: [PATCH v2] fork: check exit_signal passed in clone3() call

[Christian Brauner]

On Wed, Sep 11, 2019 at 03:52:36PM +0200, Christian Brauner wrote:
> On Wed, Sep 11, 2019 at 06:48:52AM -0700, Andrew Morton wrote:
> > On Tue, 10 Sep 2019 18:58:52 +0100 Eugene Syromiatnikov <esyr@xxxxxxxxxx> wrote:
> > 
> > > Previously, higher 32 bits of exit_signal fields were lost when
> > > copied to the kernel args structure (that uses int as a type for the
> > > respective field).  Moreover, as Oleg has noted[1], exit_signal is used
> > > unchecked, so it has to be checked for sanity before use; for the legacy
> > > syscalls, applying CSIGNAL mask guarantees that it is at least non-negative;
> > > however, there's no such thing is done in clone3() code path, and that can
> > > break at least thread_group_leader.
> > > 
> > > Checking user-passed exit_signal against ~CSIGNAL mask solves both
> > > of these problems.
> > > 
> > > [1] https://lkml.org/lkml/2019/9/10/467
> > > 
> > > * kernel/fork.c (copy_clone_args_from_user): Fail with -EINVAL if
> > > args.exit_signal has bits set outside CSIGNAL mask.
> > > (_do_fork): Note that exit_signal is expected to be checked for the
> > > sanity by the caller.
> > > 
> > > Fixes: 7f192e3cd316 ("fork: add clone3")
> > 
> > What are the user-visible runtime effects of this bug?
> > 
> > Relatedly, should this fix be backported into -stable kernels?  If so, why?
> 
> No, as I said in my other mail clone3() is not in any released kernel
> yet. clone3() is going to be released in v5.3.

Sigh, I spoke to soon... Hm, this is placed in _do_fork(). There's a
chance that this might be visible in legacy clone if anyone passes in an
invalid signal greater than NSIG right now somehow, they'd now get
EINVAL if I'm seeing this right.

So an alternative might be to only fix this in clone3() only right now
and get this patch into 5.3 to not release clone3() with this bug from
legacy clone duplicated.
And we defer the actual legacy clone fix until after next merge window
having it stew in linux-next for a couple of rcs. Distros often pull in
rcs so if anyone notices a regression for legacy clone we'll know about
it... valid_signal() checks at process exit time when the parent is
supposed to be notifed will catch faulty signals anyway so it's not that
big of a deal.

Christian