On Mon, Aug 24, 2020 at 7:03 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
>
> On Wed, Aug 19, 2020 at 06:37:25PM -0700, Peter Collingbourne wrote:
> > On Wed, Aug 19, 2020 at 8:40 AM Dave Martin <Dave.Martin@xxxxxxx> wrote:
> > >
> > > On Mon, Aug 17, 2020 at 08:33:50PM -0700, Peter Collingbourne wrote:
> > > > 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_ignored_bits{,_mask} fields.
> > > >
> > > > A new sigcontext flag, SA_XFLAGS, 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.
> > > >
> > > > Ideally this field could have just been named si_flags, but that
> > > > name was already taken by ia64, so a different name was chosen.
> > > >
> > > > Alternatively, we may consider making ia64's si_flags a generic field
> > > > and having it appear at the end of _sigfault (in the same place as
> > > > this patch has si_xflags) on non-ia64, keeping it in the same place
> > > > on ia64. ia64's si_flags is a 32-bit field with only one flag bit
> > > > allocated, so we would have 31 bits to use if we do this.
> > >
> > > For clarity, is the new si_xflags field supposed to be valid for all
> > > signal types, or just certain signals and si_codes?
> >
> > It is intended to be valid for all signal types that use the _sigfault
> > union member of siginfo. As listed in siginfo.h these are: SIGILL,
> > SIGFPE, SIGSEGV, SIGBUS, SIGTRAP, SIGEMT.
>
> SIGSYS is similar to SIGILL, is that included also?
I think that SIGSYS is covered by a separate _sigsys union member.
> > > What happens for things like a rt_sigqueueinfo() from userspace?
> >
> > Hmm. Let's enumerate each of these things, which I believe are all of
> > the call sites of the function copy_siginfo_from_user and related
> > functions (correct me if I'm wrong):
> >
> > - ptrace(PTRACE_SETSIGINFO)
> > - pidfd_send_signal
> > - rt_sigqueueinfo
> > - rt_tgsigqueueinfo
> >
> > We can handle the last three by observing that the kernel forbids
> > sending a signal with these syscalls if si_code >= 0, so we can say
> > that the value of si_xflags is only valid if si_code >= 0.
>
> Hmmm, that's what the code says (actually >= 0 or SI_TKILL), but it's
> illogical. Those are user signals, so there's no obvious reason why
> userspace shouldn't be allowed to generate their siginfo. It would
> probably be better for the kernel to police si_pid etc. in the SI_USER
> and SI_TKILL cases rather than flatly refusing, but I guess that's a
> discussion for another day.
>
> I guess the combination of SI_FROMKERNEL() and the signal number being a
> known fault signal if probably sufficient for now.
In v10 I ended up adding a comment saying that si_xflags is only valid
if 0 <= si_code < SI_KERNEL (the SI_KERNEL part was due to my
discovery of kernel code that was calling force_sig(SIGSEGV) where
force_sig uses the _kill union member). Your comment about SI_USER
made me realize that is not exactly true (since kill and
pidfd_send_signal can send a fault signal with si_code == SI_USER). I
was not aware of the SI_FROMKERNEL() macro. In v11 I will update the
comment to say that SI_FROMKERNEL(si) && si->si_code != SI_KERNEL must
be true in order for si_xflags to be valid.
> It might be helpful to have a helper to identify fault signals, but we
> don't have this today, and it's unlikely that a new kind of fault signal
> will crop up any time soon.
>
> Handlers that handle specific signal types won't care, but debuggers and
> generic backtracer code would have to be hand-hacked to add new kinds of
> fault signal today -- not a huge priority though, and orthogonal to this
> series.
>
> > As for the first one, it's more tricky. Arguably something like a
> > debugger should be able to send arbitrary signals to a debuggee, and
> > there's no reason why it shouldn't be able to set si_xflags in
> > siginfo, but on the other hand who knows how existing debuggers end up
> > setting this field today. Maybe all that we can do is have the kernel
> > clear si_xflags if it detects that the signal uses _sigfault, and let
> > si_xflags aware debuggers opt out of this behavior, perhaps by
> > introducing a PTRACE_SETSIGINFO2 or something.
>
> Most likely a debugger usually amends an existing siginfo from a trapped
> signal than generating a new one from scratch.
Right, but it could have copied the fields by hand from a
kernel-supplied siginfo before amending it.
> Given the other things that ptrace can do to the target process I don't
> think we need to police here, but your suggestion about a
> PTRACE_SETSIGINFO2 or similar, and zeroing this field by default with
> PTRACE_SETSIGINFO, does make sense.
Ack. I've implemented that in v10.
Peter
