signal: allow the null signal in rt_sigqueueinfo()
diff mbox series

Message ID 20190105054729.40397-1-cai@lca.pw
State In Next
Commit ef8d826a3ce72b299c5026532458c4d9c60aeb73
Headers show
Series
  • signal: allow the null signal in rt_sigqueueinfo()
Related show

Commit Message

Qian Cai Jan. 5, 2019, 5:47 a.m. UTC
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.

Signed-off-by: Qian Cai <cai@lca.pw>
---
 kernel/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Morton Jan. 7, 2019, 11:03 p.m. UTC | #1
On Sat,  5 Jan 2019 00:47:29 -0500 Qian Cai <cai@lca.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.
Qian Cai Jan. 8, 2019, 1:33 a.m. UTC | #2
On 1/7/19 6:03 PM, Andrew Morton wrote:
> On Sat,  5 Jan 2019 00:47:29 -0500 Qian Cai <cai@lca.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?  

I suppose you meant sig (signo) instead of si_code which is this patch is for.
Sig can never be -1 because it is unsigned int. si_code is an int which is fine
to be -1.

in /include/uapi/asm-generic/siginfo.h,

/*
 * si_code values
 * Digital reserves positive values for kernel-generated signals.
 */
#define SI_USER		0
#define SI_KERNEL	0x80
#define SI_QUEUE	-1
#define SI_TIMER	-2
#define SI_MESGQ	-3
#define SI_ASYNCIO	-4
#define SI_SIGIO	-5
#define SI_TKILL	-6
#define SI_DETHREAD	-7
#define SI_ASYNCNL	-60

> 
> - 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.
> 

Well, the thing here is that signo 0 is a valid input, so it has to process as
further as possible for error checking if I read it correctly.

in man rt_sigqueueinfo,

"As with kill(2), the null signal (0) can be used to check if the specified
process or thread exists."

Then, in man 2 kill

"If sig is 0, then no signal is sent, but error checking is still performed;
this can be used to check for the existence of a process ID or process group ID."

Later, it will will be dealt with properly in group_send_sig_info()

if (!ret && sig)
	ret = do_send_sig_info(sig, info, p, type);

return ret;

Hence the only problem here is that sig_specific_sicodes(sig) forgot to deal
with sig 0 in the first place.

Patch
diff mbox series

diff --git a/kernel/signal.c b/kernel/signal.c
index e1d7ad8e6ab1..970bb36837a9 100644
--- 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;
 		}