| Message ID | 1492712234-4950-2-git-send-email-kristina.martsenko@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kristina, 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 ... > 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 made a simple patch to fix this (using the address argument and explicitly passing the PC in when we fault on an invalid instruction). 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. 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? Cheers, Andre. > As a second bug, if cache maintenance (correctly) fails on an invalid > tagged address, the address gets passed into arm64_notify_segfault, > where find_vma fails to find the VMA due to the tag, and the wrong > si_code may be sent as part of the siginfo_t of the segfault. With this > patch, the correct si_code is sent. > > Fixes: 7dd01aef0557 ("arm64: trap userspace "dc cvau" cache operation on errata-affected core") > Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> > --- > > Note that patch #3 would also fix the first bug (incorrect segfault), > but not the second (wrong si_code), hence this patch. > > arch/arm64/kernel/traps.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c > index e52be6aa44ee..45c8eca951bc 100644 > --- a/arch/arm64/kernel/traps.c > +++ b/arch/arm64/kernel/traps.c > @@ -443,7 +443,7 @@ int cpu_enable_cache_maint_trap(void *__unused) > } > > #define __user_cache_maint(insn, address, res) \ > - if (untagged_addr(address) >= user_addr_max()) { \ > + if (address >= user_addr_max()) { \ > res = -EFAULT; \ > } else { \ > uaccess_ttbr0_enable(); \ > @@ -469,7 +469,7 @@ static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs) > int crm = (esr & ESR_ELx_SYS64_ISS_CRM_MASK) >> ESR_ELx_SYS64_ISS_CRM_SHIFT; > int ret = 0; > > - address = pt_regs_read_reg(regs, rt); > + address = untagged_addr(pt_regs_read_reg(regs, rt)); > > switch (crm) { > case ESR_ELx_SYS64_ISS_CRM_DC_CVAU: /* DC CVAU, gets promoted */ >
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
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index e52be6aa44ee..45c8eca951bc 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -443,7 +443,7 @@ int cpu_enable_cache_maint_trap(void *__unused) } #define __user_cache_maint(insn, address, res) \ - if (untagged_addr(address) >= user_addr_max()) { \ + if (address >= user_addr_max()) { \ res = -EFAULT; \ } else { \ uaccess_ttbr0_enable(); \ @@ -469,7 +469,7 @@ static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs) int crm = (esr & ESR_ELx_SYS64_ISS_CRM_MASK) >> ESR_ELx_SYS64_ISS_CRM_SHIFT; int ret = 0; - address = pt_regs_read_reg(regs, rt); + address = untagged_addr(pt_regs_read_reg(regs, rt)); switch (crm) { case ESR_ELx_SYS64_ISS_CRM_DC_CVAU: /* DC CVAU, gets promoted */
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. 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. As a second bug, if cache maintenance (correctly) fails on an invalid tagged address, the address gets passed into arm64_notify_segfault, where find_vma fails to find the VMA due to the tag, and the wrong si_code may be sent as part of the siginfo_t of the segfault. With this patch, the correct si_code is sent. Fixes: 7dd01aef0557 ("arm64: trap userspace "dc cvau" cache operation on errata-affected core") Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com> --- Note that patch #3 would also fix the first bug (incorrect segfault), but not the second (wrong si_code), hence this patch. arch/arm64/kernel/traps.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)