| Message ID | 20260824203740.3689400-2-Jeremy.Jean@oss.cyber.gouv.fr |
|---|---|
| State | New |
| Headers | show |
| Series | ARC: check user addresses in unaligned access emulation | expand |
On 8/24/26 13:37, Jérémy Jean wrote: > ARC700 raises an alignment exception before checking access permissions. > Consequently, a userspace load or store using a misaligned kernel address > reaches misaligned_fixup() before the processor rejects it. How do you know this for sure. Did you run into this issue yourself and were able to prove this ordering. And even if you found some documentation can you confirm this to be happening on real ARC700 hardware. I've been maintaining ARC forever and even do I don't have access to working ARC700 silicon. I can sympathize with your need to send a fix and it might actually be correct. But I can't take a fix that theoretically fixes something w/o demonstrating what real life case it caters so. So Nack, unless you have one of the valid reasons above. -Vineet > misaligned_fixup() decodes the instruction and repeats the access using > byte loads or stores. These accesses run in kernel mode, and exception > tables only handle accesses that fault. A mapped kernel address is > therefore read or written with supervisor permissions. > > Use access_ok() to reject addresses outside the user range before calling > the helpers. Check the whole 2- or 4-byte range and use the checked address > for the emulated operation. > > Fixes: 2e651ea1596b ("ARC: Unaligned access emulation") > Assisted-by: Codex:gpt-daybreak-blue > Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr> > --- > arch/arc/kernel/unaligned.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/arch/arc/kernel/unaligned.c b/arch/arc/kernel/unaligned.c > index 3b2d8b1bd271..f6079dc89a6d 100644 > --- a/arch/arc/kernel/unaligned.c > +++ b/arch/arc/kernel/unaligned.c > @@ -133,6 +133,8 @@ int no_unaligned_warning __read_mostly = 1; /* Only 1 warning by default */ > static void fixup_load(struct disasm_state *state, struct pt_regs *regs, > struct callee_regs *cregs) > { > + unsigned long address; > + unsigned int size; > int val; > > /* register write back */ > @@ -143,10 +145,15 @@ static void fixup_load(struct disasm_state *state, struct pt_regs *regs, > state->src2 = 0; > } > > + address = state->src1 + state->src2; > + size = state->zz ? 2 : 4; > + if (!access_ok((void __user *)address, size)) > + goto fault; > + > if (state->zz == 0) { > - get32_unaligned_check(val, state->src1 + state->src2); > + get32_unaligned_check(val, address); > } else { > - get16_unaligned_check(val, state->src1 + state->src2); > + get16_unaligned_check(val, address); > > if (state->x) > val = (val << 16) >> 16; > @@ -163,6 +170,9 @@ fault: state->fault = 1; > static void fixup_store(struct disasm_state *state, struct pt_regs *regs, > struct callee_regs *cregs) > { > + unsigned long address; > + unsigned int size; > + > /* register write back */ > if ((state->aa == 1) || (state->aa == 2)) { > set_reg(state->wb_reg, state->src2 + state->src3, regs, cregs); > @@ -181,11 +191,16 @@ static void fixup_store(struct disasm_state *state, struct pt_regs *regs, > } > } > > + address = state->src2 + state->src3; > + size = state->zz ? 2 : 4; > + if (!access_ok((void __user *)address, size)) > + goto fault; > + > /* write fix-up */ > if (!state->zz) > - put32_unaligned_check(state->src1, state->src2 + state->src3); > + put32_unaligned_check(state->src1, address); > else > - put16_unaligned_check(state->src1, state->src2 + state->src3); > + put16_unaligned_check(state->src1, address); > > return; >
Hello, On 2026-08-25 05:00, Vineet Gupta wrote: > How do you know this for sure. Did you run into this issue yourself and > were able to prove this ordering. > And even if you found some documentation can you confirm this to be > happening on real ARC700 hardware. > I've been maintaining ARC forever and even do I don't have access to > working ARC700 silicon. I cannot say I know this for sure, and I neither have access to real hardware to check further. > I can sympathize with your need to send a fix and it might actually be > correct. > But I can't take a fix that theoretically fixes something w/o > demonstrating what real life case it caters so. I have no need to send anything: I'm simply raising this potential problem to you, with a possible fix as help. In my understanding, these lists are audited by experts like you to consider public reports that may have concrete impact. I'm totally fine if this does not fit. Thanks for your time. Regards, Jérémy
diff --git a/arch/arc/kernel/unaligned.c b/arch/arc/kernel/unaligned.c index 3b2d8b1bd271..f6079dc89a6d 100644 --- a/arch/arc/kernel/unaligned.c +++ b/arch/arc/kernel/unaligned.c @@ -133,6 +133,8 @@ int no_unaligned_warning __read_mostly = 1; /* Only 1 warning by default */ static void fixup_load(struct disasm_state *state, struct pt_regs *regs, struct callee_regs *cregs) { + unsigned long address; + unsigned int size; int val; /* register write back */ @@ -143,10 +145,15 @@ static void fixup_load(struct disasm_state *state, struct pt_regs *regs, state->src2 = 0; } + address = state->src1 + state->src2; + size = state->zz ? 2 : 4; + if (!access_ok((void __user *)address, size)) + goto fault; + if (state->zz == 0) { - get32_unaligned_check(val, state->src1 + state->src2); + get32_unaligned_check(val, address); } else { - get16_unaligned_check(val, state->src1 + state->src2); + get16_unaligned_check(val, address); if (state->x) val = (val << 16) >> 16; @@ -163,6 +170,9 @@ fault: state->fault = 1; static void fixup_store(struct disasm_state *state, struct pt_regs *regs, struct callee_regs *cregs) { + unsigned long address; + unsigned int size; + /* register write back */ if ((state->aa == 1) || (state->aa == 2)) { set_reg(state->wb_reg, state->src2 + state->src3, regs, cregs); @@ -181,11 +191,16 @@ static void fixup_store(struct disasm_state *state, struct pt_regs *regs, } } + address = state->src2 + state->src3; + size = state->zz ? 2 : 4; + if (!access_ok((void __user *)address, size)) + goto fault; + /* write fix-up */ if (!state->zz) - put32_unaligned_check(state->src1, state->src2 + state->src3); + put32_unaligned_check(state->src1, address); else - put16_unaligned_check(state->src1, state->src2 + state->src3); + put16_unaligned_check(state->src1, address); return;
ARC700 raises an alignment exception before checking access permissions. Consequently, a userspace load or store using a misaligned kernel address reaches misaligned_fixup() before the processor rejects it. misaligned_fixup() decodes the instruction and repeats the access using byte loads or stores. These accesses run in kernel mode, and exception tables only handle accesses that fault. A mapped kernel address is therefore read or written with supervisor permissions. Use access_ok() to reject addresses outside the user range before calling the helpers. Check the whole 2- or 4-byte range and use the checked address for the emulated operation. Fixes: 2e651ea1596b ("ARC: Unaligned access emulation") Assisted-by: Codex:gpt-daybreak-blue Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr> --- arch/arc/kernel/unaligned.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)