Re: + kernel-fork-validate-exit_signal-in-clone-syscall.patch added to mm-nonmm-unstable branch

[Oleg Nesterov <oleg@redhat.com>]

On 03/09, Deepanshu Kartikey wrote:
>
> > Well, kernel_clone() has more users which doesn't validate .exit_signal,
> > say sys_ia32_clone().
> >
> > we need to move the valid_signal() check from copy_clone_args_from_user()
> > to kernel_clone() or copy_process()...
> >
> > So. This should fix my
> >
> >         [PATCH] do_notify_parent: sanitize the valid_signal() checks
> >         https://lore.kernel.org/all/aZsfg0Y055yuAvsq@redhat.com/
> >
> > do_notify_parent-sanitize-the-valid_signal-checks.patch in -mm tree.
> >
> > Somehow I was very sure that copy_process() paths already have the valid_signal()
> > check but my memory fooled me.
> >
> > But this is a user visible change which can cause other bug reports...
> > Perhaps we should revert do_notify_parent-sanitize-the-valid_signal-checks.patch
> > and this patch?
> >
> > Even if I think that the new valid_signal() check "fixes" the undocumented
> > behaviour, unlikely there is a sane application which passes non-valid exit
> > signal to sys_clone(). But who knows...
> >
> > Oleg.
> >
>
> Hi Oleg,
>
> Thank you for the review.
>
> You are correct that fixing only the clone() syscall is incomplete.
> sys_ia32_clone() and other kernel_clone() callers would remain
> unprotected. I will send a v2 with the valid_signal() check moved
> to kernel_clone() to cover all callers.
>
> Regarding your do_notify_parent patch — since v2 will fix the root
> cause at kernel_clone(), your patch in the -mm tree can be dropped.

Well. my patch can be dropped with or without your v2. It is just
a cleanup and I still think it is a good cleanup...

But without your fix it is wrong. From the changelog:

	The "sig" argument of do_notify_parent() must always be valid

this is not true because (contrary to what I thought) sys_clone doesn't
validate exit_signal. The

	WARN_ON_ONCE(!valid_signal(sig))

added by that patch allowed to notice the problem you are trying to fix.

But again, this (your patch) is a user-visible change, and I am worried
even if I think this change is good.

Oleg.