[PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Angelos Oikonomopoulos]

do_alignment_t32_to_handler only fixes up alignment faults for specific
instructions; it returns NULL otherwise. When that's the case, signal to
the caller that it needs to proceed with the regular alignment fault
handling (i.e. SIGBUS).

Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
---
 arch/arm64/kernel/compat_alignment.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
index deff21bfa680..76cf1c1b5bc6 100644
--- a/arch/arm64/kernel/compat_alignment.c
+++ b/arch/arm64/kernel/compat_alignment.c
@@ -34,7 +34,7 @@
 
 #define REGMASK_BITS(i)	(i & 0xffff)
 
-#define BAD_INSTR 	0xdeadc0de
+#define BAD_INSTR	0xdeadc0de
 
 /* Thumb-2 32 bit format per ARMv7 DDI0406A A6.3, either f800h,e800h,f800h */
 #define IS_T32(hi16) \
@@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
 		return 1;
 	}
 
+	if (!handler)
+		return 1;
 	type = handler(addr, instr, regs);
 
 	if (type == TYPE_ERROR || type == TYPE_FAULT)
-- 
2.49.0

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Catalin Marinas]

On Wed, Mar 26, 2025 at 02:35:21PM +0100, Angelos Oikonomopoulos wrote:
> do_alignment_t32_to_handler only fixes up alignment faults for specific
> instructions; it returns NULL otherwise. When that's the case, signal to
> the caller that it needs to proceed with the regular alignment fault
> handling (i.e. SIGBUS).

Did you hit this in practice? Which instruction triggered the alignment
fault that was not handled by do_alignment_t32_to_handler()? Standard
LDR/STR should not trigger unaligned accesses unless you have some
device memory mapped in user space.

> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
> ---
>  arch/arm64/kernel/compat_alignment.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
> index deff21bfa680..76cf1c1b5bc6 100644
> --- a/arch/arm64/kernel/compat_alignment.c
> +++ b/arch/arm64/kernel/compat_alignment.c
> @@ -34,7 +34,7 @@
>  
>  #define REGMASK_BITS(i)	(i & 0xffff)
>  
> -#define BAD_INSTR 	0xdeadc0de
> +#define BAD_INSTR	0xdeadc0de

Unrelated change (white space I guess), please drop it, not worth
fixing.

-- 
Catalin

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Angelos Oikonomopoulos]

On Fri Mar 28, 2025 at 8:06 PM CET, Catalin Marinas wrote:
> On Wed, Mar 26, 2025 at 02:35:21PM +0100, Angelos Oikonomopoulos wrote:
>> do_alignment_t32_to_handler only fixes up alignment faults for specific
>> instructions; it returns NULL otherwise. When that's the case, signal to
>> the caller that it needs to proceed with the regular alignment fault
>> handling (i.e. SIGBUS).
>
> Did you hit this in practice? Which instruction triggered the alignment
> fault that was not handled by do_alignment_t32_to_handler()? Standard
> LDR/STR should not trigger unaligned accesses unless you have some
> device memory mapped in user space.

Yah, I've hit this in practice. The offending instruction was an ldrex
to an unaligned address, while running 32-bit code on an "Ampere(R)
Altra(R) Processor Q80-30 CPU @ 3.0GHz". Fixing the unaligned access in
the program is one thing, but this resulted in multiple oopses in CI.

>>  #define REGMASK_BITS(i)	(i & 0xffff)
>>  
>> -#define BAD_INSTR 	0xdeadc0de
>> +#define BAD_INSTR	0xdeadc0de
>
> Unrelated change (white space I guess), please drop it, not worth
> fixing.

That snuck past me in an amend, will send a v2.

Thanks,
Angelos

[PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Angelos Oikonomopoulos]

do_alignment_t32_to_handler only fixes up alignment faults for specific
instructions; it returns NULL otherwise. When that's the case, signal to
the caller that it needs to proceed with the regular alignment fault
handling (i.e. SIGBUS).

Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
---
 arch/arm64/kernel/compat_alignment.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
index deff21bfa680..b68e1d328d4c 100644
--- a/arch/arm64/kernel/compat_alignment.c
+++ b/arch/arm64/kernel/compat_alignment.c
@@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
 		return 1;
 	}
 
+	if (!handler)
+		return 1;
 	type = handler(addr, instr, regs);
 
 	if (type == TYPE_ERROR || type == TYPE_FAULT)
-- 
2.49.0

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Anshuman Khandual]

On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
> do_alignment_t32_to_handler only fixes up alignment faults for specific
> instructions; it returns NULL otherwise. When that's the case, signal to
> the caller that it needs to proceed with the regular alignment fault
> handling (i.e. SIGBUS).
> 
> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
> ---
>  arch/arm64/kernel/compat_alignment.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
> index deff21bfa680..b68e1d328d4c 100644
> --- a/arch/arm64/kernel/compat_alignment.c
> +++ b/arch/arm64/kernel/compat_alignment.c
> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>  		return 1;
>  	}
>  
> +	if (!handler)
> +		return 1;

do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
the right thing to do here and consistent. Otherwise does this cause a
kernel crash during subsequent call into handler() ?

>  	type = handler(addr, instr, regs);
>  
>  	if (type == TYPE_ERROR || type == TYPE_FAULT)

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Angelos Oikonomopoulos]

On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
> On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
>> do_alignment_t32_to_handler only fixes up alignment faults for specific
>> instructions; it returns NULL otherwise. When that's the case, signal to
>> the caller that it needs to proceed with the regular alignment fault
>> handling (i.e. SIGBUS).
>> 
>> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
>> ---
>>  arch/arm64/kernel/compat_alignment.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>> index deff21bfa680..b68e1d328d4c 100644
>> --- a/arch/arm64/kernel/compat_alignment.c
>> +++ b/arch/arm64/kernel/compat_alignment.c
>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>  		return 1;
>>  	}
>>  
>> +	if (!handler)
>> +		return 1;
>
> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
> the right thing to do here and consistent. Otherwise does this cause a
> kernel crash during subsequent call into handler() ?

Yes. We call a NULL pointer so we Oops.

Angelos

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Anshuman Khandual]

On 4/1/25 12:28, Angelos Oikonomopoulos wrote:
> On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
>> On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
>>> do_alignment_t32_to_handler only fixes up alignment faults for specific
>>> instructions; it returns NULL otherwise. When that's the case, signal to
>>> the caller that it needs to proceed with the regular alignment fault
>>> handling (i.e. SIGBUS).
>>>
>>> Signed-off-by: Angelos Oikonomopoulos <angelos@igalia.com>
>>> ---
>>>  arch/arm64/kernel/compat_alignment.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>>> index deff21bfa680..b68e1d328d4c 100644
>>> --- a/arch/arm64/kernel/compat_alignment.c
>>> +++ b/arch/arm64/kernel/compat_alignment.c
>>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>>  		return 1;
>>>  	}
>>>  
>>> +	if (!handler)
>>> +		return 1;
>>
>> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
>> the right thing to do here and consistent. Otherwise does this cause a
>> kernel crash during subsequent call into handler() ?
> 
> Yes. We call a NULL pointer so we Oops.

Then the commit message should have the kernel Oops splash dump and also
might need to have Fixes: and CC: stable tags etc ?

Also wondering if handler return value should be checked inside the switch
block just after do_alignment_t32_to_handler() assignment.

	handler = do_alignment_t32_to_handler()
	if (!handler)
		return 1

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Angelos Oikonomopoulos]

On Tue Apr 1, 2025 at 9:47 AM CEST, Anshuman Khandual wrote:
>
>
> On 4/1/25 12:28, Angelos Oikonomopoulos wrote:
>> On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
>>> On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
[...]
>>>>  arch/arm64/kernel/compat_alignment.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>>>> index deff21bfa680..b68e1d328d4c 100644
>>>> --- a/arch/arm64/kernel/compat_alignment.c
>>>> +++ b/arch/arm64/kernel/compat_alignment.c
>>>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>>>  		return 1;
>>>>  	}
>>>>  
>>>> +	if (!handler)
>>>> +		return 1;
>>>
>>> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
>>> the right thing to do here and consistent. Otherwise does this cause a
>>> kernel crash during subsequent call into handler() ?
>> 
>> Yes. We call a NULL pointer so we Oops.
>
> Then the commit message should have the kernel Oops splash dump and also
> might need to have Fixes: and CC: stable tags etc ?

Sure, I can add those. Thanks for the suggestions!

> Also wondering if handler return value should be checked inside the switch
> block just after do_alignment_t32_to_handler() assignment.
>
> 	handler = do_alignment_t32_to_handler()
> 	if (!handler)
> 		return 1

I can see the appeal of that, but I think placing the check right before
the single dereference is a more future-proof fix, in that it reduce the
chances that a later patch will re-introduce a potential NULL pointer
dereference.

Angelos

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Anshuman Khandual]

On 4/1/25 13:39, Angelos Oikonomopoulos wrote:
> On Tue Apr 1, 2025 at 9:47 AM CEST, Anshuman Khandual wrote:
>>
>>
>> On 4/1/25 12:28, Angelos Oikonomopoulos wrote:
>>> On Tue Apr 1, 2025 at 8:05 AM CEST, Anshuman Khandual wrote:
>>>> On 3/31/25 14:24, Angelos Oikonomopoulos wrote:
> [...]
>>>>>  arch/arm64/kernel/compat_alignment.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kernel/compat_alignment.c b/arch/arm64/kernel/compat_alignment.c
>>>>> index deff21bfa680..b68e1d328d4c 100644
>>>>> --- a/arch/arm64/kernel/compat_alignment.c
>>>>> +++ b/arch/arm64/kernel/compat_alignment.c
>>>>> @@ -368,6 +368,8 @@ int do_compat_alignment_fixup(unsigned long addr, struct pt_regs *regs)
>>>>>  		return 1;
>>>>>  	}
>>>>>  
>>>>> +	if (!handler)
>>>>> +		return 1;
>>>>
>>>> do_alignment_t32_to_handler() could return NULL, returning 1 seems to be
>>>> the right thing to do here and consistent. Otherwise does this cause a
>>>> kernel crash during subsequent call into handler() ?
>>>
>>> Yes. We call a NULL pointer so we Oops.
>>
>> Then the commit message should have the kernel Oops splash dump and also
>> might need to have Fixes: and CC: stable tags etc ?
> 
> Sure, I can add those. Thanks for the suggestions!
> 
>> Also wondering if handler return value should be checked inside the switch
>> block just after do_alignment_t32_to_handler() assignment.
>>
>> 	handler = do_alignment_t32_to_handler()
>> 	if (!handler)
>> 		return 1
> 
> I can see the appeal of that, but I think placing the check right before
> the single dereference is a more future-proof fix, in that it reduce the
> chances that a later patch will re-introduce a potential NULL pointer
> dereference.

Makes sense. A small nit - just wondering if the following restructuring
here would make things bit more readable ? Regardless, your decision.

	if (handler)
        	type = handler(addr, instr, regs);
        else
                return 1;
> 
> Angelos
> 

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Angelos Oikonomopoulos]

On Tue Apr 1, 2025 at 10:22 AM CEST, Anshuman Khandual wrote:
> Makes sense. A small nit - just wondering if the following restructuring
> here would make things bit more readable ? Regardless, your decision.
>
> 	if (handler)
>         	type = handler(addr, instr, regs);
>         else
>                 return 1;

I went with the original formulation since -- to my mind at least -- an
"early exit" idiom feels more appropriate for something we found we
can't handle. Could work either way though.

Thanks,
Angelos

Re: [PATCH] arm64: Don't call NULL in do_compat_alignment_fixup

[Will Deacon]

On Tue, Apr 01, 2025 at 10:58:48AM +0200, Angelos Oikonomopoulos wrote:
> On Tue Apr 1, 2025 at 10:22 AM CEST, Anshuman Khandual wrote:
> > Makes sense. A small nit - just wondering if the following restructuring
> > here would make things bit more readable ? Regardless, your decision.
> >
> > 	if (handler)
> >         	type = handler(addr, instr, regs);
> >         else
> >                 return 1;
> 
> I went with the original formulation since -- to my mind at least -- an
> "early exit" idiom feels more appropriate for something we found we
> can't handle. Could work either way though.

What you have is fine.

Will