linux-kernel - Re: [PATCH] signal: allow the null signal in rt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20190107150336.4fc75d2aa20b637a259e50b3@linux-foundation.org>
Date:   Mon, 7 Jan 2019 15:03:36 -0800
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Qian Cai <cai@....pw>
Cc:     linux-kernel@...r.kernel.org, Oleg Nesterov <oleg@...hat.com>
Subject: Re: [PATCH] signal: allow the null signal in rt_sigqueueinfo()

On Sat,  5 Jan 2019 00:47:29 -0500 Qian Cai <cai@....pw> 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.