On Tue, Nov 3, 2020 at 10:33 AM Catalin Marinas <catalin.marinas@xxxxxxx> wrote:
>
> On Mon, Nov 02, 2020 at 08:09:44PM -0800, Peter Collingbourne wrote:
> > The kernel currently clears the tag bits (i.e. bits 56-63) in the fault
> > address exposed via siginfo.si_addr and sigcontext.fault_address. However,
> > the tag bits may be needed by tools in order to accurately diagnose
> > memory errors, such as HWASan [1] or future tools based on the Memory
> > Tagging Extension (MTE).
> >
> > We should not stop clearing these bits in the existing fault address
> > fields, because there may be existing userspace applications that are
> > expecting the tag bits to be cleared. Instead, create a new pair of
> > union fields in siginfo._sigfault, and store the tag bits of FAR_EL1
> > there, together with a mask specifying which bits are valid.
>
> This comment is slightly confusing as they are not union fields in
> _sigfault.
Good catch, I will remove the word "union" here.
> > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h
> > index f43778355b77..2b2ed0394457 100644
> > --- a/include/uapi/asm-generic/siginfo.h
> > +++ b/include/uapi/asm-generic/siginfo.h
> > @@ -94,6 +94,8 @@ union __sifields {
> > void *_pad[6];
> > };
> > __u32 _faultflags;
> > + unsigned long _addr_tag_bits;
> > + unsigned long _addr_tag_bits_mask;
> > } _sigfault;
>
> Can we not actually add these as a struct to the union? Do we expect the
> other fields to be valid?
The idea is that the fields would be available for all si_codes,
including SEGV_MAPERR (which is important as I mention below). There's
no fundamental reason why the information shouldn't be made available
for BUS_MCEERR_*, SEGV_BNDERR or SEGV_PKUERR either, or other union
members that we may introduce in the future, just because they happen
to use a union member. Although we could retroactively add a union
member for currently non-union si_codes, and add new fields to the
current union members, that would basically be the same thing as
adding the new struct fields that I am adding here.
> Also there's a 32-bit gap I mentioned on the previous patch between
> _faultflags and _addr_tag_bits.
As I mentioned on the previous patch, I think the gap is unavoidable.
> That said, I wonder whether we could solve this for MTE without new
> fields by always setting the tag in si_addr when si_code is SEGV_MTE*.
This wouldn't solve the problem for MTE in the case where there is a
non-linear buffer overflow that extends into an unmapped page, in
which case we would get a SEGV_MAPERR that we would still need the tag
bits for.
> Alternatively, we could add a prctl() bit to require tagged si_addr.
It's an option that we considered but I would be concerned about the
compatibility implications of this. In practice, on Android we would
always have this bit set, so applications would be exposed to the tag
bits in si_addr. If applications have previously relied on the
documented behavior that the tag bits are unset, they may get confused
by them now being set. It also wouldn't provide a way for the kernel
to communicate which tag bits are valid.
Peter
> Well, I don't mind the _addr_tag_bits* fields if they are part of the
> union and keep si_addr intact.
>
> --
> Catalin
