On Tue, Apr 13, 2021 at 04:52:34PM +0000, Liam Howlett wrote:

* Catalin Marinas [off-list ref] [210412 13:44]:

quoted

On Wed, Apr 07, 2021 at 03:11:06PM +0000, Liam Howlett wrote:

quoted

find_vma() will continue to search upwards until the end of the virtual
memory space.  This means the si_code would almost never be set to
SEGV_MAPERR even when the address falls outside of any VMA.  The result
is that the si_code is not reliable as it may or may not be set to the
correct result, depending on where the address falls in the address
space.

Using find_vma_intersection() allows for what is intended by only
returning a VMA if it falls within the range provided, in this case a
window of 1.

Signed-off-by: Liam R. Howlett <redacted>
---
 arch/arm64/kernel/traps.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index a05d34f0e82a..a44007904a64 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c

@@ -383,9 +383,10 @@ void force_signal_inject(int signal, int code, unsigned long address, unsigned i
 void arm64_notify_segfault(unsigned long addr)
 {
 	int code;
+	unsigned long ut_addr = untagged_addr(addr);
 
 	mmap_read_lock(current->mm);
-	if (find_vma(current->mm, untagged_addr(addr)) == NULL)
+	if (find_vma_intersection(current->mm, ut_addr, ut_addr + 1) == NULL)
 		code = SEGV_MAPERR;
 	else
 		code = SEGV_ACCERR;

[...]

quoted

I don't think your change is entirely correct either. We can have a
fault below the vma of a stack (with VM_GROWSDOWN) and
find_vma_intersection() would return NULL but it should be a SEGV_ACCERR
instead.

I'm pretty sure I am missing something.  From what you said above, I
think this means that there can be a user cache fault below the stack
which should notify the user application that they are not allowed to
expand the stack by sending a SIGV_ACCERR in the si_code?  Is this
expected behaviour or am I missing a code path to this function?

My point was that find_vma() may return a valid vma where addr < vm_end
but also addr < vm_addr. It's the responsibility of the caller to check
that that vma can be expanded (VM_GROWSDOWN) and we do something like
this in __do_page_fault(). find_vma_intersection(), OTOH, requires addr

= vm_start.

If we hit this case (addr < vm_start), normally we'd first need to check
whether it's expandable and, if not, return MAPERR. If it's expandable,
it should be ACCERR since something else caused the fault.

Now, I think at least for user_cache_maint_handler(), we can assume that
__do_page_fault() handled any expansion already, so we don't need to
check it here. In this case, your find_vma_intersection() check should
work.

Are there other cases where we invoke arm64_notify_segfault() without a
prior fault? I think in swp_handler() we can bail out early before we
even attempted the access so we may report MAPERR but ACCERR is a better
indication. Also in sys_rt_sigreturn() we always call it as
arm64_notify_segfault(regs->sp). I'm not sure that's correct in all
cases, see restore_altstack().

I guess this code needs some tidying up.

quoted

Maybe this should employ similar checks as __do_page_fault() (with
expand_stack() and VM_GROWSDOWN).

You mean the code needs to detect endianness and to check if this is an
attempt to expand the stack for both cases?

Nothing to do with endianness, just the relation between the address and
the vma->vm_start and whether the vma can be expanded down.

-- 
Catalin

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