Messages in this thread | | | Subject | Re: [PATCH] signal: allow the null signal in rt_sigqueueinfo() | From | Qian Cai <> | Date | Mon, 7 Jan 2019 20:33:20 -0500 |
| |
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.
| |