[PATCH 1/4] arm64: traps: fix userspace cache maintenance emulation on a tagged pointer

[Kristina Martsenko]

Hi Andre,

On 21/04/17 11:59, Andre Przywara wrote:
> On 20/04/17 19:17, Kristina Martsenko wrote:
>> When we emulate userspace cache maintenance in the kernel, we can
>> currently send the task a SIGSEGV even though the maintenance was done
>> on a valid address. This happens if the address has a non-zero address
>> tag, and happens to not be mapped in.
>>
>> When we get the address from a user register, we don't currently remove
>> the address tag before performing cache maintenance on it. If the
>> maintenance faults, we end up in either __do_page_fault, where find_vma
>> can't find the VMA if the address has a tag, or in do_translation_fault,
>> where the tagged address will appear to be above TASK_SIZE. In both
>> cases, the address is not mapped in, and the task is sent a SIGSEGV.
> 
> Right, well spotted!
> So thanks for the patch, which I think is correct. But ...

Thanks for taking a look.

>> This patch removes the tag from the address before using it. With this
>> patch, the fault is handled correctly, the address gets mapped in, and
>> the cache maintenance succeeds.
> 
> Looking more closely at this code, I see that we actually don't use the
> address parameter in the force_signal_inject() function. Instead we
> always put the PC address into the siginfo structure, which is wrong in
> case this SEGV is triggered by an invalid address of a cache maintenance
> operation.

I agree this is a bug in existing code, as it means we currently put a
different address into the siginfo structure when we emulate cache
maintenance, compared to when we don't emulate it.

I think the bug is independent of this series though, and the fix should
be sent as a separate patch/series, as it is not blocking this series
and doesn't involve address tags.

> I made a simple patch to fix this (using the address argument and
> explicitly passing the PC in when we fault on an invalid instruction).

Sounds about right to me. Although I noticed that swp emulation also
goes through arm64_notify_segfault. I think the behaviour for swp
emulation should stay the same as in arch/arm/ set_segfault(), i.e.
faulting address used in find_vma, but PC passed in siginfo. Unless
that's a bug in arch/arm/ (for a non-emulated swp, they seem to pass the
faulting address in siginfo instead).

> But now we would pass the untagged address back into userland. I am not
> sure this is a real problem, since we don't promise anything in case of
> tagged pointers, if I got this correctly.

Yes, as documented in Documentation/arm64/tagged-pointers.txt, we do not
guarantee that the tag is preserved when delivering a signal.

> But also our untagged_addr() macro seems to not cover all cases
> correctly, for instance passing in 0x00ffffffffff5678 (which is an
> invalid, but untagged address) would extend to some probably valid
> kernel pointer. And although this would fail our user space address
> check, we would return a wrong address (with all the upper bits being 1)
> in siginfo.
> 
> Do we care about this?
> What would be the best fix for the untagged_addr macro? Is that macro
> actually the proper place to fix this issue?

I don't know if we care, but personally I think that if
force_signal_inject is fixed, then untagged_addr should be fixed along
with it. I think the macro is the right place to fix it, by only sign
extending if bit 55 is 0. That way it will turn a tagged address into an
untagged address, and will not change a non-tagged address. (This is
also what clear_address_tag in patch #3 of this series does.)

Thanks,
Kristina