Subject: Re: [PATCH v8] arm64: Expose FAR_EL1 tag bits in siginfo

From: Dave Martin <[email-masked]>
Date: Tue, 18 Aug 2020 14:50:13 +0100

On Mon, Aug 17, 2020 at 08:16:42PM -0700, Peter Collingbourne wrote:
> On Tue, Jul 14, 2020 at 10:36 AM Dave Martin <[email-masked]> wrote:

[...]

> > As a final random idea to add to the mix, we could add two or more
> > flags in sa_flags, and require the kernel to transform them in a
> > specific way, say:
> >
> > #define SA_WANT_FLAGS 0x00c700000
> > #define SA_HAVE_FLAGS 0x009200000
> > #define SA_FLAGS_MASK 0x00ff00000
> >
> > volatile sig_atomic_t have_flags = 0;
> >
> >         sa.sa_flags |= SA_WANT_FLAGS;
> >         if (sigaction(n, &sa, NULL))
> >                 if (!sigaction(n, NULL, &sa) &&
> >                                 (sa.sa_flags & SA_FLAGS_MASK) == SA_HAVE_FLAGS)
> >                         have_flags = 1;
> >
> > This is at least proof against "dumb readback".
> >
> > Provided that the handler can cope with the have_flags == 0 case and
> > just reads the flag once per call, I don't think we would need to worry
> > about races.
> >
> > Of course, an interceptor that doesn't understand this mechanism and
> > munges or manufactures its own siginfo might still fail to properly
> > initialise our new field before passing it on to a signal handler that
> > is expecting it.  But that's already broken: such an interceptor might
> > also not understand new si_codes that the client code absolutely relies
> > on.  And new si_codes _do_ get added (that's another extensibility fail
> > in the existing signal API).
> >
> >
> > So... overall, maybe a bit in the signal number isn't a lot worse, and
> > perhaps it _will_ lead to cleaner failures.
> >
> > Really, I don't see a way to solve it properly without a new API.
> 
> I started on implementing my signal number bit idea, and in the
> process of doing so came up with another idea that may be better from
> the "don't abuse existing arguments" perspective. It involves a
> sigaction protocol similar to the one that you describe above, but it
> only requires one new bit (plus one bit per new flag) so it is less
> wasteful of sa_flags bits.
> 
> The idea is twofold:
> 
> 1. Require the kernel to clear unknown flag bits in sa_flags when
> passing them back in oldact. I suppose that this is technically a
> behavior change for sigaction, but critically, this change in behavior
> only applies to unallocated flags, which we are free to change the
> meaning of. We can simply define each existing unallocated flag bit to
> mean "clear this bit in oldact (unless the bit becomes supported in
> the future)". There was already code doing something similar in a
> limited fashion on x86, which we can remove by using this approach.

Sounds reasonable.  It's quite hard to imagine how software could
accidentally rely on unallocated sa_flags bits reading back out
unmodified through sigaction().  Software that deliberately relies on
this for some bit that is never allocated would be obviously non POSIX
compliant.

> 2. Define a flag bit SA_UNSUPPORTED which will never be supported by
> the kernel. Now userspace can use the fact that the bit has been
> cleared to mean that it can trust that other unsupported bits have
> also been cleared.
> 
> Now we may have code like this:
> 
> #define SA_UNSUPPORTED 0x400
> #define SA_XFLAGS 0x800
> 
> volatile sig_atomic_t have_xflags = 0;
> 
>          sa.sa_flags |= SA_UNSUPPORTED | SA_XFLAGS;
>          if (sigaction(n, &sa, NULL))
>                  if (!sigaction(n, NULL, &sa) &&
>                                  !(sa.sa_flags & SA_UNSUPPORTED) &&
> (sa.sa_flags & SA_XFLAGS))
>                          have_xflags = 1;

OK, so I think the novelty here is that we detect support by requiring
the kernel to clear one bit while preserving another.  That does indeed
seem more robust: an old kernel (unless bizarrely buggy) would either
clear all unsupported bits or preserve them all.  Other OSes that extend
their sigaction() in line with POSIX would also be highly unlikely to
exhibit this behaviour by accident IMHO, making it easier for this to
coexist with other people's extensions.

Interceptors still may not transparently work with this approach, but I
think that's a reasonable price to pay.

> 
> > In the meantime, can I suggest:
> >
> >  (1) Come up with an extensible way of encoding supplementary
> >      information in siginfo.  If the consensus is that zeroing unused
> >      fields is sufficient and that the kernel and compiler will
> >      reliably do it, then great.  Otherwise, we might need explicit
> >      flags fields or something.
> 
> I thought about this for a while and concluded that we probably want a
> flags field anyway. si_addr_ignored_bits is something of a special
> case in the sense that we can define the zero value to mean
> "unknown" by taking advantage of the mask field (which I suppose is
> something of a flags field), but we can't necessarily say that the
> same is true for any fields that we may add in the future. For
> example, if we wanted to communicate whether the failing access is a
> read or a write, we would need a tristate: read, write and "unknown"
> (and arrange for old kernels' behavior to be interpreted as
> "unknown"). If we rely on zeroing then we may implement this by adding
> a field like:
> 
> char si_access_type; // 0 = unknown, 1 = read, 2 = write
> 
> But that's really just a (slightly wasteful, because we use the entire
> byte) flags field, so we may as well define an actual flags field to
> begin with and let people add their flags there.
> 
> Unfortunately we can't name it sa_flags because ia64 got there first.
> We may consider making the ia64 field generic though (ia64 only uses
> one bit of their field, so we would have 31 free bits). In the
> meantime, I added a separate field, sa_xflags.

Seems fair enough to me.  We still have the option to use zeroing for
detection of fields where it works.

Would SA_XFLAGS imply zeroing of unallocated siginfo fields?  This may
just be a matter of documentation, if the kernel already does the
zeroing today.

> 
> >  (2) Hack up any simple mechanism (such as your signal number flag) for
> >      requesting/detecting the extra information.
> >
> > Along with an illustration of a application of the mechanism (i.e.,
> > reporting address tag bits), this should at least provide a basis for
> > further review.
> >
> > We can then try to swap in a different mechanism for (2) if people have
> > still have concerns (or it not, keep it).
> 
> Sounds good. Apologies for not replying sooner, I was hoping that Eric
> would chime in so that I would get a sense of which approach he would
> prefer (so that I wouldn't spend as much time implementing in an
> undesired direction), then this fell off my radar. I decided to go
> with the SA_UNSUPPORTED approach that I mentioned above for now, and
> I'll send a v9 with that implemented shortly. Most of the change is
> about letting the architecture-independent code know which bits are
> supported, so it should be easy to replace the detection mechanism
> with another idea like the signal number bit.

No worries, I think various people have had distractions (I certainly
have ... but I digress).

I'll take a look at your v9.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel