Re: [PATCH v14 8/8] arm64: expose FAR_EL1 tag bits in siginfo

[ebiederm@xmission.com (Eric W. Biederman)]

Peter Collingbourne <pcc@google.com> writes:

> On Tue, Nov 10, 2020 at 7:12 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Peter Collingbourne <pcc@google.com> writes:
>>
>> > On Mon, Nov 9, 2020 at 5:13 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> >>
>> >> Peter Collingbourne <pcc@google.com> writes:
>> >>
>> >> > 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
>> >> > fields in siginfo._sigfault, and store the tag bits of FAR_EL1 there,
>> >> > together with a mask specifying which bits are valid.
>> >> >
>> >> > A flag is added to si_faultflags to allow userspace to determine whether
>> >> > the values in the fields are valid.
>> >>
>> >> I think I am missing some things:
>> >>
>> >> Today it is documented that the tag bits are cleared, and so we can't
>> >> use the highbits to hold the tag bits by default.
>> >>
>> >> Why do you need to deliver which tag bits are valid?  That feels like an
>> >> implementation detail that is needed to setup the tag bits.  It feels
>> >> like it would be constant per process.  So I don't understand why the
>> >> siginfo needs to report information the process should already have.
>> >
>> > It isn't constant as it may vary depending on the specific type of
>> > fault being delivered. For example on arm64 the architecture only
>> > provides us with bits 56-59 of the tag for tag check faults, while all
>> > other data aborts also provide bits 60-63. Now although the user
>> > program may distinguish the two cases by checking the si_code, we
>> > would also like to allow future architecture extensions to provide
>> > bits 60-63 for tag check faults as well and allow distinguishing
>> > between "bits 60-63 were zero" and "bits 60-63 were unknown" (which is
>> > important when providing error reports).
>>
>> Does that mean that bits 60-63 are effectively unusable as tag bits
>> if the tag check fault won't report them?
>
> The hardware doesn't support tag checking on bits 60-63, only on bits
> 56-59, so in terms of hardware enforced memory tag checking they can't
> be used as tag bits. But they are still subject to address tagging aka
> top-byte-ignore so they could be used by userspace for other purposes
> (e.g. if the allocator adds entropy to all 8 bits instead of just bits
> 56-59 they could in theory be used to allow better diagnostics with
> more precise matching of an invalid access to a previous allocation).
>
>> If you can use bits 60-63 as tag bits how does that work if they aren't
>> reported?
>
> It still works but we're limited in how many allocations we can match
> accesses to (16 vs 256).
>
>> > I suppose that you could imagine that, if the "bits 60-63 available"
>> > extension ever comes to pass, it could be exposed via a bit in
>> > getauxval(AT_HWCAP2) (or maybe something like AT_HWCAP3 depending on
>> > how long it takes to arrive) and that would provide a way for
>> > userspace to know which bits are valid. But it seems like allowing
>> > this to vary per signal is relatively cheap, allows the information to
>> > be made available trivially architecture independently and keeps our
>> > options open for the future (because we don't know if some future
>> > architecture will actually make this a per-signal attribute).
>> >
>> > That being said, maybe we could solve this problem at the point at
>> > which we do encounter such an architecture though.
>>
>> My goal would be to extend things to the minimum extent necessary to
>> handle today's reality well.  Usually architectures evolve in
>> unanticipated directions so simple and straight forward usually wins for
>> handling future evolution.  As there is simply less old baggage to carry
>> around.
>>
>> However I don't understand if reporting the valid bits on a signal by
>> signal basis has a real advantage today or not.  If it helps today we
>> will find room for the field.
>
> Okay, that makes sense. With the architecture as specified today I
> don't think we would need it, since you can write a function that
> operates on the siginfo and tells you what the si_addr_tag_bits_mask
> would be without it.
>
>> >> Want prevents adding a sigaction sa_flag SA_EXPOSE_TABITS that when set
>> >> causes the high bits to be set, and when clear (the default) will have
>> >> the signal delivery code clear those bits.
>> >>
>> >> That should be enough for code that wants the tag bits to ask for them.
>> >> As userspace would need to be updated to get the new bits
>> >>
>> >> Even if you have chained handlers.  The chaining mechanism would need to
>> >> be updated and it could call the aware handlers first then clear the tag
>> >> bits and call the rest of the handlers.
>> >>
>> >> It feels like always passing the tag bits in the address and then
>> >> clearing them in the copy to userspace if the signal handler is
>> >> not ready for them would be easier to maintain.
>> >
>> > I think that approach might work. Although it may make life harder for
>> > callers of ptrace(PTRACE_SETSIGINFO) since they may need to know the
>> > value of the bit in order to prepare a correct siginfo structure, if
>> > we can reasonably expect them to always be delivering an exact copy of
>> > a signal that was received before then maybe that is okay.
>>
>> I think we can reasonably expect callers of PTRACE_SETSIGINFO to be able
>> to either deal the full reality of what is going on, or to only generate
>> signals that they fully understand.
>>
>> Other than the use by CRIU it is a debugging facility and it is not
>> expected for ordinary usage.  The non-CRIU use case would really seem to
>> be what happens if I inject arbitrary signal X into process Y.  For that
>> you need the ability to inject an arbitrary signal.
>>
>> My real sense with PTRACE_SETSIGINFO is that if we wind up with a
>> regression we can deal with it then.
>
> Okay, that works for me.
>
>> > Assuming that this is an alternative to introducing
>> > si_addr_tag_bits_mask, the userspace code would need to use the flag
>> > bit support detection protocol for SA_EXPOSE_TAGBITS in order to be
>> > able to distinguish between "no bits valid" and "some bits valid", and
>> > then use an architecture-specific mechanism to determine exactly which
>> > bits are valid. Is that okay for a generic feature?
>>
>> Unless I am mistaken setting the bits is already architecture specific
>> so having some architecture specific code in there should not be a big
>> problem.
>>
>> But I really don't understand the arm case well enough to know if we can
>> get away without si_addr_tag_bits_mask, and in turn without the flags
>> field that indicates the si_addr_tag_bits_mask is present.
>>
>> So I am asking questions so I can understand just what we get from
>> si_addr_tag_bits_mask.
>
> If we allow the derivation of the mask to be architecture-specific
> (which I'd be comfortable with) then I don't think we need it. What we
> would end up with is:
>
> - The tag bits mask is constant and architecturally defined (in
> arm64's case it would be 0xff << 56).
> - The valid tag bits mask specifies which bits of the tag bits mask in
> the fault address are propagated to si_addr. It is defined in an
> architecture-specific way based on the signal handler's
> SA_EXPOSE_TAGBITS bit, the contents of siginfo and possibly global
> information such as getauxval(AT_HWCAP*).
>   - on arm64 this would currently be defined as:
>     - if SA_EXPOSE_TAGBITS is not set then the mask is 0xff << 56 for
> SIGTRAP/TRAP_BRKPT, 0 otherwise
>     - if SA_EXPOSE_TAGBITS is set then the mask is 0xf << 56 for
> SIGSEGV/SEGV_MTESERR, 0xff << 56 otherwise
> - If SA_EXPOSE_TAGBITS is set then the bits in the tag bits mask that
> are not also in the valid tag bits mask have an undefined value (this
> would e.g. allow future expansion of arm64 to expose bits 60-63 on tag
> check faults).
> - If the kernel does not support SA_EXPOSE_TAGBITS (as determined
> using the flag bit support detection protocol) then the behavior is as
> if SA_EXPOSE_TAGBITS is not set.
>
> So I think I'd be fine with dropping it but let me experiment with the
> new approach so that I can confirm that it's practical on Android and
> I'll get back to you.

Sounds like a plan.

Eric


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel