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

[Dave Martin <Dave.Martin@arm.com>]

On Wed, Nov 11, 2020 at 02:15:15PM -0600, Eric W. Biederman wrote:
> Dave Martin <Dave.Martin@arm.com> writes:
> 
> > On Mon, Nov 09, 2020 at 07:57:33PM -0600, Eric W. Biederman wrote:
> >> Peter Collingbourne <pcc@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?

Cheers
---Dave