- 11 Oct, 2018 1 commit
-
-
Eric W. Biederman authored
The bounds checks in known_siginfo_layout only guards against positive numbers that are too large, large negative can slip through and can cause out of bounds accesses. Ordinarily this is not a concern because early in signal processing the signal number is filtered with valid_signal which ensures it is a small positive signal number, but copy_siginfo_from_user is called before this check is performed. [ 73.031126] BUG: unable to handle kernel paging request at ffffffff6281bcb6 [ 73.032038] PGD 3014067 P4D 3014067 PUD 0 [ 73.032565] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC PTI [ 73.033287] CPU: 0 PID: 732 Comm: trinity-c3 Tainted: G W T 4.19.0-rc1-00077-g4ce5f9c9 #1 [ 73.034423] RIP: 0010:copy_siginfo_from_user+0x4d/0xd0 [ 73.034908] Code: 00 8b 53 08 81 fa 80 00 00 00 0f 84 90 00 00 00 85 d2 7e 2d 48 63 0b 83 f9 1f 7f 1c 8d 71 ff bf d8 04 01 50 48 0f a3 f7 73 0e <0f> b6 8c 09 20 bb 81 82 39 ca 7f 15 eb 68 31 c0 83 fa 06 7f 0c eb [ 73.036665] RSP: 0018:ffff88001b8f7e20 EFLAGS: 00010297 [ 73.037160] RAX: 0000000000000000 RBX: ffff88001b8f7e90 RCX: fffffffff00000cb [ 73.037865] RDX: 0000000000000001 RSI: 00000000f00000ca RDI: 00000000500104d8 [ 73.038546] RBP: ffff88001b8f7e80 R08: 0000000000000000 R09: 0000000000000000 [ 73.039201] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000008 [ 73.039874] R13: 00000000000002dc R14: 0000000000000000 R15: 0000000000000000 [ 73.040613] FS: 000000000104a880(0000) GS:ffff88001f000000(0000) knlGS:0000000000000000 [ 73.041649] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 73.042405] CR2: ffffffff6281bcb6 CR3: 000000001cb52003 CR4: 00000000001606b0 [ 73.043351] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 73.044286] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600 [ 73.045221] Call Trace: [ 73.045556] __x64_sys_rt_tgsigqueueinfo+0x34/0xa0 [ 73.046199] do_syscall_64+0x1a4/0x390 [ 73.046708] ? vtime_user_enter+0x61/0x80 [ 73.047242] ? __context_tracking_enter+0x4e/0x60 [ 73.047714] ? __context_tracking_enter+0x4e/0x60 [ 73.048278] entry_SYSCALL_64_after_hwframe+0x44/0xa9 Therefore fix known_siginfo_layout to take an unsigned signal number instead of a signed signal number. All valid signal numbers are small positive numbers so they will not be affected, but invalid negative signal numbers will now become large positive signal numbers and will not be used as indices into the sig_sicodes array. Making the signal number unsigned makes it difficult for similar mistakes to happen in the future. Fixes: 4ce5f9c9 ("signal: Use a smaller struct siginfo in the kernel") Inspired-by:
Sean Christopherson <sean.j.christopherson@intel.com> Reported-by:
kernel test robot <rong.a.chen@intel.com> Signed-off-by:
"Eric W. Biederman" <ebiederm@xmission.com>
-
- 08 Oct, 2018 1 commit
-
-
Eric W. Biederman authored
Andrei Vagin <avagin@gmail.com> reported: > Accoding to the man page, the user should not set si_signo, it has to be set > by kernel. > > $ man 2 rt_sigqueueinfo > > The uinfo argument specifies the data to accompany the signal. This > argument is a pointer to a structure of type siginfo_t, described in > sigaction(2) (and defined by including <sigaction.h>). The caller > should set the following fields in this structure: > > si_code > This must be one of the SI_* codes in the Linux kernel source > file include/asm-generic/siginfo.h, with the restriction that > the code must be negative (i.e., cannot be SI_USER, which is > used by the kernel to indicate a signal sent by kill(2)) and > cannot (since Linux 2.6.39) be SI_TKILL (which is used by the > kernel to indicate a signal sent using tgkill(2)). > > si_pid This should be set to a process ID, typically the process ID of > the sender. > > si_uid This should be set to a user ID, typically the real user ID of > the sender. > > si_value > This field contains the user data to accompany the signal. For > more information, see the description of the last (union sigval) > argument of sigqueue(3). > > Internally, the kernel sets the si_signo field to the value specified > in sig, so that the receiver of the signal can also obtain the signal > number via that field. > > On Tue, Sep 25, 2018 at 07:19:02PM +0200, Eric W. Biederman wrote: >> >> If there is some application that calls sigqueueinfo directly that has >> a problem with this added sanity check we can revisit this when we see >> what kind of crazy that application is doing. > > > I already know two "applications" ;) > > https://github.com/torvalds/linux/blob/master/tools/testing/selftests/ptrace/peeksiginfo.c > https://github.com/checkpoint-restore/criu/blob/master/test/zdtm/static/sigpending.c > > Disclaimer: I'm the author of both of them. Looking at the kernel code the historical behavior has alwasy been to prefer the signal number passed in by the kernel. So sigh. Implmenet __copy_siginfo_from_user and __copy_siginfo_from_user32 to take that signal number and prefer it. The user of ptrace will still use copy_siginfo_from_user and copy_siginfo_from_user32 as they do not and never have had a signal number there. Luckily this change has never made it farther than linux-next. Fixes: e75dc036 ("signal: Fail sigqueueinfo if si_signo != sig") Reported-by:
Andrei Vagin <avagin@gmail.com> Tested-by:
-
- 03 Oct, 2018 6 commits
-
-
Eric W. Biederman authored
We reserve 128 bytes for struct siginfo but only use about 48 bytes on 64bit and 32 bytes on 32bit. Someday we might use more but it is unlikely to be anytime soon. Userspace seems content with just enough bytes of siginfo to implement sigqueue. Or in the case of checkpoint/restart reinjecting signals the kernel has sent. Reducing the stack footprint and the work to copy siginfo around from 2 cachelines to 1 cachelines seems worth doing even if I don't have benchmarks to show a performance difference. Suggested-by:
Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by:
-
Eric W. Biederman authored
Linus recently observed that if we did not worry about the padding member in struct siginfo it is only about 48 bytes, and 48 bytes is much nicer than 128 bytes for allocating on the stack and copying around in the kernel. The obvious thing of only adding the padding when userspace is including siginfo.h won't work as there are sigframe definitions in the kernel that embed struct siginfo. So split siginfo in two; kernel_siginfo and siginfo. Keeping the traditional name for the userspace definition. While the version that is used internally to the kernel and ultimately will not be padded to 128 bytes is called kernel_siginfo. The definition of struct kernel_siginfo I have put in include/signal_types.h A set of buildtime checks has been added to verify the two structures have the same field offsets. To make it easy to verify the change kernel_siginfo retains the same size as siginfo. The reduction in size comes in a following change. Signed-off-by: -
Eric W. Biederman authored
In preparation for using a smaller version of siginfo in the kernel introduce copy_siginfo_from_user and use it when siginfo is copied from userspace. Make the pattern for using copy_siginfo_from_user and copy_siginfo_from_user32 to capture the return value and return that value on error. This is a necessary prerequisite for using a smaller siginfo in the kernel than the kernel exports to userspace. Signed-off-by: -
Eric W. Biederman authored
Rework the defintion of struct siginfo so that the array padding struct siginfo to SI_MAX_SIZE can be placed in a union along side of the rest of the struct siginfo members. The result is that we no longer need the __ARCH_SI_PREAMBLE_SIZE or SI_PAD_SIZE definitions. Signed-off-by: -
Eric W. Biederman authored
The kernel needs to validate that the contents of struct siginfo make sense as siginfo is copied into the kernel, so that the proper union members can be put in the appropriate locations. The field si_signo is a fundamental part of that validation. As such changing the contents of si_signo after the validation make no sense and can result in nonsense values in the kernel. As such simply fail if someone is silly enough to set si_signo out of sync with the signal number passed to sigqueueinfo. I don't expect a problem as glibc's sigqueue implementation sets "si_signo = sig" and CRIU just returns to the kernel what the kernel gave to it. If there is some application that calls sigqueueinfo directly that has a problem with this added sanity check we can revisit this when we see what kind of crazy that application is doing. Signed-off-by: -
Eric W. Biederman authored
When moving all of the architectures specific si_codes into siginfo.h, I apparently overlooked EMT_TAGOVF. Move it now. Remove the now redundant test in siginfo_layout for SIGEMT as now NSIGEMT is always defined. Signed-off-by:
-
- 27 Sep, 2018 27 commits
-
-
Eric W. Biederman authored
Signed-off-by: -
Eric W. Biederman authored
Pass the signal number, and the signal code, and the faulting address into uc32_notify_die so the callers do not need to generate a struct siginfo. In ucs32_ntoify_die use the newly passed in information to call force_sig_fault to generate the siginfo and send the error. This simplifies the code making the chances of bugs much less likely. Signed-off-by: -
Eric W. Biederman authored
Signed-off-by: -
Eric W. Biederman authored
Acked-by:
Vineet Gupta <vgupta@synopsys.com> Signed-off-by:
-
Eric W. Biederman authored
Pass signr, sicode, and address into unhandled_exception as explicit parameters instead of members of struct siginfo. Then in unhandled exception generate and send the siginfo using force_sig_fault. This keeps the code simpler and less error prone. Acked-by:
-
Eric W. Biederman authored
Acked-by:
Tony Luck <tony.luck@intel.com> Signed-off-by:
-
Eric W. Biederman authored
The ia64 handling of failure to return from a signal frame has been trying to set overlapping fields in struct siginfo since 2.3.43. The si_code corresponds to the fields that were stomped (not the field that is actually written), so I can not imagine a piece of userspace code making sense of the signal frame if it looks closely. In practice failure to return from a signal frame is a rare event that almost never happens. Someone using an alternate signal stack to recover and looking in detail is even more rare. So I presume no one has ever noticed and reported this ia64 nonsense. Sort this out by causing ia64 to use force_sig(SIGSEGV) like other architectures. Fixes: 2.3.43 Cc: Tony Luck <tony.luck@intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: linux-ia64@vger.kernel.org Acked-by:
-
Eric W. Biederman authored
The ia64 handling of failure to setup a signal frame has been trying to set overlapping fields in struct siginfo since 2.3.43. The si_pid and si_uid fields are stomped when the si_addr field is set. The si_code of SI_KERNEL indicates that si_pid and si_uid should be valid, and that si_addr does not exist. Being at odds with the definition of SI_KERNEL and with nothing to indicate that this was a signal frame setup failure there is no way for userspace to know that si_addr was filled out instead. In practice failure to setup a signal frame is rare, and si_pid and si_uid are always set to 0 when si_code is SI_KERNEL so I expect no one has looked closely enough before to see this weirdness. Further the only difference between force_sigsegv_info and the generic force_sigsegv other than the return code is that force_sigsegv_info stomps the si_uid and si_pid fields. Remove the bug and simplify the code by using force_sigsegv in this case just like other architectures. Fixes: 2.3.43 Cc: Tony Luck <tony.luck@intel.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: linux-ia64@vger.kernel.org Acked-by:
-
Eric W. Biederman authored
This simplifies the code making it clearer what is going on, and making the siginfo generation easier to maintain. Signed-off-by: -
Eric W. Biederman authored
Signed-off-by: -
Eric W. Biederman authored
Signed-off-by: -
Eric W. Biederman authored
In arm_notify_die call force_sig_fault to let the generic code handle siginfo generation. This removes some boiler plate making the code easier to maintain in the long run. Signed-off-by: -
Eric W. Biederman authored
Reviewed-by:
Catalin Marinas <catalin.marinas@arm.com> Tested-by:
-
Eric W. Biederman authored
Add arm64_force_sig_ptrace_errno_trap for consistency with arm64_force_sig_fault and use it where appropriate. This adds the show_signal logic to the force_sig_errno_trap case, where it was apparently overlooked earlier. Reviewed-by:
-
Eric W. Biederman authored
This will let the description be reused shortly. Reviewed-by:
-
Eric W. Biederman authored
The function has no more callers so remove it. Reviewed-by:
-
Eric W. Biederman authored
Add arm64_force_sig_mceerr for consistency with arm64_force_sig_fault, and use it in the one location that can take advantage of it. This removes the fiddly filling out of siginfo before sending a signal reporting an memory error to userspace. Reviewed-by:
-
Eric W. Biederman authored
Wrap force_sig_fault with a helper that calls arm64_show_signal and call arm64_force_sig_fault where appropraite. Reviewed-by:
-
Eric W. Biederman authored
This code is truly common between the signal sending cases so share it. Reviewed-by:
-
Eric W. Biederman authored
As this work is truly common between all of the signal sending cases there is no need to repeat it between the different cases. Reviewed-by:
-
Eric W. Biederman authored
Not all of the signals passed to __do_user_fault can be handled the same way so expand the now tiny __do_user_fault in it's callers and remove it. Reviewed-by:
-
Eric W. Biederman authored
It gets easy to confuse what is going on when some code is shared and some not so stop sharing the trivial bits of signal generation to make future updates easier to understand. Reviewed-by:
-
Eric W. Biederman authored
These two cases are practically the same and use siginfo differently from the other signals sent from do_page_fault. So consolidate them to make future changes easier. Reviewed-by:
-
Eric W. Biederman authored
This pepares for sending signals with something other than arm64_force_sig_info. Reviewed-by:
-
Eric W. Biederman authored
Filling in siginfo is error prone and so it is wise to use more specialized helpers to do that work. Factor out the arm specific unhandled signal reporting from the work of delivering a signal so the code can be modified to use functions that take the information to fill out siginfo as parameters. Reviewed-by:
-
Eric W. Biederman authored
Every caller passes in current for tsk so there is no need to pass tsk. Instead make tsk a local variable initialized to current. Reviewed-by:
-
Eric W. Biederman authored
Instead of generating a struct siginfo before calling arm64_notify_die pass the signal number, tne sicode and the fault address into arm64_notify_die and have it call force_sig_fault instead of force_sig_info to let the generic code generate the struct siginfo. This keeps code passing just the needed information into siginfo generating code, making it easier to see what is happening and harder to get wrong. Further by letting the generic code handle the generation of struct siginfo it reduces the number of sites generating struct siginfo making it possible to review them and verify that all of the fiddly details for a structure passed to userspace are handled properly. Reviewed-by:
-
- 21 Sep, 2018 5 commits
-
-
Eric W. Biederman authored
Reviewed-by:
Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by:
-
Eric W. Biederman authored
Call force_sig_pkuerr directly instead of rolling it by hand in _exception_pkey. Reviewed-by:
-
Eric W. Biederman authored
Now that _exception no longer calls _exception_pkey it is no longer necessary to handle any signal with any si_code. All pkey exceptions are SIGSEGV with paired with SEGV_PKUERR. So just handle that case and remove the now unnecessary parameters from _exception_pkey. Reviewed-by:
-
Eric W. Biederman authored
The callers of _exception don't need the pkey exception logic because they are not processing a pkey exception. So just call exception_common directly and then call force_sig_fault to generate the appropriate siginfo and deliver the appropriate signal. Reviewed-by:
-
Eric W. Biederman authored
It is brittle and wrong to populate si_pkey when there was not a pkey exception. The field does not exist for all si_codes and in some cases another field exists in the same memory location. So factor out the code that all exceptions handlers must run into exception_common, leaving the individual exception handlers to generate the signals themselves. Reviewed-by:
-
