| Message ID | 20250331085415.122409-1-angelos@igalia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Don't call NULL in do_compat_alignment_fixup | expand |
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)
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
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
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
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 >
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
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)
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(+)