Re: [PATCH] signal: allow the null signal in rt_sigqueueinfo()

[Andrew Morton]

On Sat,  5 Jan 2019 00:47:29 -0500 Qian Cai <cai@xxxxxx> wrote:

> Running the trinity fuzzer triggered this,
> 
> UBSAN: Undefined behaviour in kernel/signal.c:2946:7
> shift exponent 4294967295 is too large for 64-bit type 'long unsigned
> int'
> [ 3752.406618]  dump_stack+0xe0/0x17a
> [ 3752.419817]  ubsan_epilogue+0xd/0x4e
> [ 3752.423429]  __ubsan_handle_shift_out_of_bounds+0x1d6/0x227
> [ 3752.447269]  known_siginfo_layout.cold.9+0x16/0x1b
> [ 3752.452105]  __copy_siginfo_from_user+0x4b/0x70
> [ 3752.466620]  do_syscall_64+0x164/0x7ea
> [ 3752.565030]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 
> This is because signo is 0 from userspace, and then it ends up calling
> (1UL << -1) in sig_specific_sicodes(). Since the null signal (0) is
> allowed in the spec, just deal with it accordingly.
> 
> ...
>
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2943,7 +2943,7 @@ static bool known_siginfo_layout(unsigned sig, int si_code)
>  	if (si_code == SI_KERNEL)
>  		return true;
>  	else if ((si_code > SI_USER)) {
> -		if (sig_specific_sicodes(sig)) {
> +		if (sig && sig_specific_sicodes(sig)) {
>  			if (si_code <= sig_sicodes[sig].limit)
>  				return true;
>  		}

Maybe.

- What happens if userspace passes in si_code == -1?  

- If we are to check the validity of the userspace-provided input
  then it would be better to do that up-front, right at the point where
  the data is copied in from userspace.  That's better than checking it
  several layers deep in one particular place which hit an issue.