[PATCH v14 7/8] signal: define the field siginfo.si_faultflags

[Eric W. Biederman]

Dave Martin <Dave.Martin at arm.com> writes:

> On Wed, Nov 11, 2020 at 02:15:15PM -0600, Eric W. Biederman wrote:
>> Dave Martin <Dave.Martin at arm.com> writes:
>> 
>> > On Mon, Nov 09, 2020 at 07:57:33PM -0600, Eric W. Biederman wrote:
>> >> Peter Collingbourne <pcc at google.com> writes:
>> >> 
>> >> > This field will contain flags that may be used by signal handlers to
>> >> > determine whether other fields in the _sigfault portion of siginfo are
>> >> > valid. An example use case is the following patch, which introduces
>> >> > the si_addr_tag_bits{,_mask} fields.
>> >> >
>> >> > A new sigcontext flag, SA_FAULTFLAGS, is introduced in order to allow
>> >> > a signal handler to require the kernel to set the field (but note
>> >> > that the field will be set anyway if the kernel supports the flag,
>> >> > regardless of its value). In combination with the previous patches,
>> >> > this allows a userspace program to determine whether the kernel will
>> >> > set the field.
>> >> >
>> >> > It is possible for an si_faultflags-unaware program to cause a signal
>> >> > handler in an si_faultflags-aware program to be called with a provided
>> >> > siginfo data structure by using one of the following syscalls:
>> >> >
>> >> > - ptrace(PTRACE_SETSIGINFO)
>> >> > - pidfd_send_signal
>> >> > - rt_sigqueueinfo
>> >> > - rt_tgsigqueueinfo
>> >> >
>> >> > So we need to prevent the si_faultflags-unaware program from causing an
>> >> > uninitialized read of si_faultflags in the si_faultflags-aware program when
>> >> > it uses one of these syscalls.
>> >> >
>> >> > The last three cases can be handled by observing that each of these
>> >> > syscalls fails if si_code >= 0. We also observe that kill(2) and
>> >> > tgkill(2) may be used to send a signal where si_code == 0 (SI_USER),
>> >> > so we define si_faultflags to only be valid if si_code > 0.
>> >> >
>> >> > There is no such check on si_code in ptrace(PTRACE_SETSIGINFO), so
>> >> > we make ptrace(PTRACE_SETSIGINFO) clear the si_faultflags field if it
>> >> > detects that the signal would use the _sigfault layout, and introduce
>> >> > a new ptrace request type, PTRACE_SETSIGINFO2, that a si_faultflags-aware
>> >> > program may use to opt out of this behavior.
>> >> 
>> >> So I think while well intentioned this is misguided.
>> >> 
>> >> gdb and the like may use this but I expect the primary user is CRIU
>> >> which simply reads the signal out of one process saves it on disk
>> >> and then restores the signal as read into the new process (possibly
>> >> on a different machine).
>> >> 
>> >> At least for the CRIU usage PTRACE_SETSIGINFO need to remain a raw
>> >> pass through kind of operation.
>> >
>> > This is a problem, though.
>> >
>> > How can we tell the difference between a siginfo that was generated by
>> > the kernel and a siginfo that was generated (or altered) by a non-xflags
>> > aware userspace?
>> >
>> > Short of revving the whole API, I don't see a simple solution to this.
>> 
>> Unlike receiving a signal.  We do know that userspace old and new
>> always sends unused fields as zero into PTRACE_SETSIGINFO.
>> 
>> The split into kernel_siginfo verifies this and fails userspace if it
>> does something different.  No problems have been reported.
>> 
>> So in the case of xflags a non-xflags aware userspace would either pass
>> the siginfo from through from somewhere else (such as
>> PTRACE_GETSIGINFO), or it would simply generate a signal with all of
>> the xflags bits clear.  So everything should work regardless.
>> 
>> > Although a bit of a hack, could we include some kind of checksum in the
>> > siginfo?  If the checksum matches during PTRACE_SETSIGINFO, we could
>> > accept the whole thing; xflags included.  Otherwise, we could silently
>> > drop non-self-describing extensions.
>> >
>> > If we only need to generate the checksum when PTRACE_GETSIGINFO is
>> > called then it might be feasible to use a strong hash; otherwise, this
>> > mechanism will be far from bulletproof.
>> >
>> > A hash has the advantage that we don't need any other information
>> > to validate it beyond a salt: if the hash matches, it's self-
>> > validating.  We could also package other data with it to describe the
>> > presence of extensions, but relying on this for regular sigaction()/
>> > signal delivery use feels too high-overhead.
>> >
>> > For debuggers, I suspect that PTRACE_SETSIGINFO2 is still useful:
>> > userspace callers that want to write an extension field that they
>> > knowingly generated themselves should have a way to express that.
>> >
>> > Thoughts?
>
> Eric, did you have any view on the hash idea here?

I am not quite certain what you meant by salt.  A per kernel instance
secret I suspect.

Such a secret would break creating siginfo by hand and checkpointing
and restoring on a different machine.

If you don't go with full crypto security it sounds like it would work.

If we really need to deploy xflags I think it bears looking at, but
right now it feels like one thing too many.



Eric