Re: [PATCH v2 2/3] arm64: mm: Handle PAN faults on uaccess CPY* instructions

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

On Mon, Mar 10, 2025 at 02:15:53PM +0000, Kristina Martsenko wrote:
> On 07/03/2025 21:37, Catalin Marinas wrote:
> > On Fri, Mar 07, 2025 at 06:53:37PM +0000, Robin Murphy wrote:
> >> On 2025-03-07 6:45 pm, Catalin Marinas wrote:
> >>> On Fri, Feb 28, 2025 at 05:00:05PM +0000, Kristina Martsenko wrote:
> >>>> +bool extable_insn_may_access_user(const struct exception_table_entry *ex,
> >>>> +				  unsigned long esr)
> >>>> +{
> >>>> +	switch (ex->type) {
> >>>> +	case EX_TYPE_UACCESS_CPY:
> >>>> +		return cpy_faulted_on_uaccess(ex, esr);
> >>>> +	default:
> >>>> +		return true;
> >>>> +	}
> >>>> +}
> >>>
> >>> Not a problem with this patch but I wonder whether we should return
> >>> false for EX_TYPE_LOAD_UNALIGNED_ZEROPAD for completeness
> >>
> >> Or maybe rather, true for EX_TYPE_UACCESS_ERR_ZERO and then false in the
> >> default case?
> > 
> > Yes.
> 
> I thought you said in an earlier (off-list) discussion that 
> EX_TYPE_KACCESS_ERR_ZERO shouldn't return false here because 
> __get_kernel_nofault() may get called with untrusted addresses? Or did I
> misunderstand?

TBH, I don't remember. Thinking about it, if we have a bug and someone
exploits __get_kernel_nofault() by giving it a user address, the above
function should indeed return false in principle. Not a big problem
since the actual user access won't happen but we may get into an
infinite loop.

I think something like this should do:

diff --git a/arch/arm64/mm/extable.c b/arch/arm64/mm/extable.c
index 6e0528831cd3..ab6775747601 100644
--- a/arch/arm64/mm/extable.c
+++ b/arch/arm64/mm/extable.c
@@ -28,10 +28,12 @@ bool insn_may_access_user(unsigned long addr, unsigned long esr)
 		return false;
 
 	switch (ex->type) {
+	case EX_TYPE_UACCESS_ERR_ZERO:
+		return true;
 	case EX_TYPE_UACCESS_CPY:
 		return cpy_faulted_on_uaccess(ex, esr);
 	default:
-		return true;
+		return false;
 	}
 }
 

-- 
Catalin