Re: [PATCH v3] arm64: Expose original FAR_EL1 value in sigcontext

[Catalin Marinas <catalin.marinas@arm.com>]

On Thu, Apr 30, 2020 at 10:59:19AM +0100, Will Deacon wrote:
> On Thu, Apr 30, 2020 at 10:50:01AM +0100, Catalin Marinas wrote:
> > On Wed, Apr 29, 2020 at 10:08:26PM +0100, Will Deacon wrote:
> > > On Fri, Mar 27, 2020 at 12:19:15PM -0700, 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 far_context in
> > > > sigcontext (similar to the existing esr_context), and store the original
> > > > value of FAR_EL1 (including the tag bits) there.
> > > > 
> > > > [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html
> > > > 
> > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > ---
> > > > v3:
> > > > - add documentation to tagged-pointers.rst
> > > > - update comments in sigcontext.h
> > > 
> > > Hmm, although the code looks fine, why don't we just expose the tag in the
> > > new field, rather than duplicate the address information? I'm nervous about
> > > exposing privileged registers directly to userspace.
> > 
> > That's for consistency with ESR_EL1 which we expose in a similar way,
> > though with bits of it not relevant to user masked out. For FAR_EL1, all
> > the bits are relevant, even if some of them are duplicated in the
> > si_addr field.
> 
> It may be consistent, but I would argue that exposing ESR_EL1 was a mistake,
> as illustrated by cc19846079a7 ("arm64: fault: Don't leak data in ESR
> context for user fault on kernel VA"). We have to live with that, but we
> should try to do better for new fields in the sigcontext.

The alternative would be to only expose the tag of the faulting address
if you are worried of leaking some kernel address from FAR_EL1 (and I
agree, there is a risk).

> > > Also, Catalin, could you elaborate on the MTE use-case please? The
> > > architecture says that FAR_EL1[63:60] are UNKNOWN on a synchronous tag
> > > check fault, so we'd have to *avoid* exposing them in that case!
> > 
> > With MTE, FAR_EL1[63:60] will be cleared on sync tag check faults (not
> > currently done as I don't have this patch in my MTE series).
> 
> Ok, but in [1] you said "I'm fine with this change for consistency and
> may help with the fault information printed by the kernel with khwasan
> or (later) MTE."
> 
> But I don't think consistency is necessarily a good thing here and I don't
> see how it helps with MTE if we zap the bits to 0! We'd be better off not
> exposing the information at all in this situation.

The plan for MTE is to only zap bits 63:60 which are unknown on tag
check fault. The actual MTE tag in 59:56 would be preserved. There is an
inconsistency with the TBI feature but that only happens for tag check
faults which is a new thing. The behaviour on any other fault will be
consistent with the non-MTE case (page faults etc.).

-- 
Catalin

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