llvm dev - Mar 2018 - [compiler-rt] Use of ESR context in AArch64 sigframe

On Wed, Mar 7, 2018 at 7:48 AM, Dmitry Vyukov <dvyukov at google.com>
wrote:
> On Wed, Mar 7, 2018 at 4:39 PM, Andrey Ryabinin <aryabinin at
virtuozzo.com> wrote:
>> On 03/06/2018 08:58 PM, Will Deacon wrote:
>>> Hi all,
>>>
>>> As part of some recent work to harden the Kernel Address Space
Layout
>>> Randomisation (KASLR) implementation in arm64 Linux, I've
proposed a
>>> patch for the kernel which omits the ESR context from the signal
frame
>>> if the faulting virtual address is outside the range of addresses
which
>>> can be mapped by userspace.
>>>
>>>
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/563837.html
>>>
>>> Looking around, it seems that AddressSanitizer is using this
information
>>> in compiler-rt in order to distinguish the faulting access type
between
>>> READ, WRITE or UNKNOWN. With this change, all attempted accesses to
kernel
>>> memory from userspace will be reported as UNKNOWN.
>>>
>>> Is this likely to cause a problem?
>>
>> I guess this shouldn't be a bid deal.
>> AFAICS compiler-rt uses this information only in diagnostic message.
>
> +address-sanitizer mailing list
>
> Hi,
>
> These diagnostic messages are then parsed and analyzed, and access
> type is used at least during automatic security pre-assessment. Being
> capable to read arbitrary memory is different from being able to write
> arbitrary memory. Though, I don't know how we treat UNKNOWN. If
it's
> the same as WRITE, then it's probably fine.
This would disable read/write detection for almost all wild pointer
dereferences, since they are more than likely to have a 1 in one of
the higher order bits. This is undesirable, but not the end of the
world. Is it possible to sanitize ESR instead of omitting it
altogether?

Also, your patch looks like it would disable ESR context for all
faults on tagged addresses, too. Since Linux unconditionally enables
TBI, it should be safe to zero out the MSB before the address check.

Thanks for the replies. Some comments below.

On Wed, Mar 07, 2018 at 11:40:41AM -0800, Evgenii Stepanov
wrote:
> On Wed, Mar 7, 2018 at 7:48 AM, Dmitry Vyukov <dvyukov at google.com>
wrote:
> > On Wed, Mar 7, 2018 at 4:39 PM, Andrey Ryabinin <aryabinin at
virtuozzo.com> wrote:
> >> On 03/06/2018 08:58 PM, Will Deacon wrote:
> >>> Hi all,
> >>>
> >>> As part of some recent work to harden the Kernel Address Space
Layout
> >>> Randomisation (KASLR) implementation in arm64 Linux, I've
proposed a
> >>> patch for the kernel which omits the ESR context from the
signal frame
> >>> if the faulting virtual address is outside the range of
addresses which
> >>> can be mapped by userspace.
> >>>
> >>>
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/563837.html
> >>>
> >>> Looking around, it seems that AddressSanitizer is using this
information
> >>> in compiler-rt in order to distinguish the faulting access
type between
> >>> READ, WRITE or UNKNOWN. With this change, all attempted
accesses to kernel
> >>> memory from userspace will be reported as UNKNOWN.
> >>>
> >>> Is this likely to cause a problem?
> >>
> >> I guess this shouldn't be a bid deal.
> >> AFAICS compiler-rt uses this information only in diagnostic
message.
> >
> > +address-sanitizer mailing list
> >
> > Hi,
> >
> > These diagnostic messages are then parsed and analyzed, and access
> > type is used at least during automatic security pre-assessment. Being
> > capable to read arbitrary memory is different from being able to write
> > arbitrary memory. Though, I don't know how we treat UNKNOWN. If
it's
> > the same as WRITE, then it's probably fine.
> 
> This would disable read/write detection for almost all wild pointer
> dereferences, since they are more than likely to have a 1 in one of
> the higher order bits. This is undesirable, but not the end of the
> world. Is it possible to sanitize ESR instead of omitting it
> altogether?
What do you do with the read/write information for a totally wild pointer
like this?

Sanitising is an option on the table -- we could fake up something like a
level 0 translation fault, I'm just wary of faking things up if we could
avoid the problem altogether (hence this email).

It's also worth noting that the WnR bit needs special treatment for things
like atomic instructions and cache maintenance instructions which currently
appears to be missing. Would it be to bad to look at si_addr to spot kernel
addresses and handle them specially?
> Also, your patch looks like it would disable ESR context for all
> faults on tagged addresses, too. Since Linux unconditionally enables
> TBI, it should be safe to zero out the MSB before the address check.
The tag is currently removed as part of the early exception entry code,
so it won't propagate this far.

Thanks,

Will

On Thu, Mar 8, 2018 at 9:11 AM, Will Deacon <will.deacon at arm.com>
wrote:
> Thanks for the replies. Some comments below.
>
> On Wed, Mar 07, 2018 at 11:40:41AM -0800, Evgenii Stepanov wrote:
>> On Wed, Mar 7, 2018 at 7:48 AM, Dmitry Vyukov <dvyukov at
google.com> wrote:
>> > On Wed, Mar 7, 2018 at 4:39 PM, Andrey Ryabinin <aryabinin at
virtuozzo.com> wrote:
>> >> On 03/06/2018 08:58 PM, Will Deacon wrote:
>> >>> Hi all,
>> >>>
>> >>> As part of some recent work to harden the Kernel Address
Space Layout
>> >>> Randomisation (KASLR) implementation in arm64 Linux,
I've proposed a
>> >>> patch for the kernel which omits the ESR context from the
signal frame
>> >>> if the faulting virtual address is outside the range of
addresses which
>> >>> can be mapped by userspace.
>> >>>
>> >>>
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/563837.html
>> >>>
>> >>> Looking around, it seems that AddressSanitizer is using
this information
>> >>> in compiler-rt in order to distinguish the faulting access
type between
>> >>> READ, WRITE or UNKNOWN. With this change, all attempted
accesses to kernel
>> >>> memory from userspace will be reported as UNKNOWN.
>> >>>
>> >>> Is this likely to cause a problem?
>> >>
>> >> I guess this shouldn't be a bid deal.
>> >> AFAICS compiler-rt uses this information only in diagnostic
message.
>> >
>> > +address-sanitizer mailing list
>> >
>> > Hi,
>> >
>> > These diagnostic messages are then parsed and analyzed, and access
>> > type is used at least during automatic security pre-assessment.
Being
>> > capable to read arbitrary memory is different from being able to
write
>> > arbitrary memory. Though, I don't know how we treat UNKNOWN.
If it's
>> > the same as WRITE, then it's probably fine.
>>
>> This would disable read/write detection for almost all wild pointer
>> dereferences, since they are more than likely to have a 1 in one of
>> the higher order bits. This is undesirable, but not the end of the
>> world. Is it possible to sanitize ESR instead of omitting it
>> altogether?
>
> What do you do with the read/write information for a totally wild pointer
> like this?
>
> Sanitising is an option on the table -- we could fake up something like a
> level 0 translation fault, I'm just wary of faking things up if we
could
> avoid the problem altogether (hence this email).
>
> It's also worth noting that the WnR bit needs special treatment for
things
> like atomic instructions and cache maintenance instructions which currently
> appears to be missing. Would it be to bad to look at si_addr to spot kernel
> addresses and handle them specially?
We use this for preliminary classification of bugs, on the assumption
that bad writes are scarier than bad reads. We don't care about
si_addr permissions, what matters is the type of access requested by
the faulting instruction, i.e. PC at the time of the fault. Worst
case, we could disassemble the instruction to find out if its a read
or a write, and on arm64 it is not even that hard.
>
>> Also, your patch looks like it would disable ESR context for all
>> faults on tagged addresses, too. Since Linux unconditionally enables
>> TBI, it should be safe to zero out the MSB before the address check.
>
> The tag is currently removed as part of the early exception entry code,
> so it won't propagate this far.
That's good to know.