Messages in this thread | | | Date | Wed, 11 Sep 2019 16:16:36 +0200 | From | Christian Brauner <> | Subject | Re: [PATCH v2] fork: check exit_signal passed in clone3() call |
| |
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@redhat.com> 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
| |