Re: [External] Re: [PATCH] riscv: Fix SR_SUM leakage across context switches

[yunhui cui]

Hi Andy,

On Fri, Oct 23, 2026 at 6:19 AM Andy Chiu <tchiu@xxxxxxxxxxxxxxx> wrote:
>
> Hi Yunhui,
>
> On Thu, Oct 22, 2026 at 04:32:12PM +0800, Yunhui Cui wrote:
> > __switch_to() saves the sstatus value of prev and restores the SR_SUM bit
> > of next with csrs. However, csrs only sets bits. If prev has SR_SUM set
> > and next has SR_SUM clear in its saved state, the old SR_SUM bit remains
> > set in sstatus.
> >
> > This lets next run with user memory access enabled even though its saved
> > SR_SUM state is clear.
> >
> > Use csrrc to save the sstatus value of prev and clear SR_SUM from the
> > live CSR before restoring the saved SR_SUM state of next. This keeps
> > prev's SR_SUM state intact, while preventing it from leaking to next.
>
> Do you have a way to trigger this bug on existing code or is it the cae
> where the bug could happen but we never experienced it in real life?
>
> If I understand correctly, this can only happen when the user access
> routing volutarily makes a schedule() call mid-flight, or a code fogets
> to turn off SR_SUM right after an user access and has a live SR_SUM when
> calling schedule some time after. Both case should be a bug as SR_SUM
> shouldn't live in the kernel code that is not doing user access.
>
> A trap-introduced context switch would interrupt a code that have SR_SUM
> set. However, since the bit is cleared at every exception entry, the
> context switch code would not see a live SR_SUM. Seeing the bit set in
> this case means something is not turning SR_SUM off properly.
>
> I aggree that this is the better way to handle SR_SUM in a context switch
> but attaching a call trace or stating that "We have not caught this in
> real world due to ..." in the commit message would be appreciated.
>

I don't think we need a "better way" to justify this. This is a bug in
__switch_to(). One possible path to hit it is
user_write_access_begin() ... local_bh_enable().

Thanks，
Yunhui

> Cheers,
> Andy
>
> >
> > Fixes: 788aa64c01f1 ("riscv: save the SR_SUM status over switches")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>
> > ---
> >  arch/riscv/kernel/entry.S | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > index 61b7a77166971..628b85e3915a7 100644
> > --- a/arch/riscv/kernel/entry.S
> > +++ b/arch/riscv/kernel/entry.S
> > @@ -443,15 +443,15 @@ SYM_FUNC_START(__switch_to)
> >       REG_S s10, TASK_THREAD_S10_RA(a3)
> >       REG_S s11, TASK_THREAD_S11_RA(a3)
> >
> > -     /* save the user space access flag */
> > -     csrr  s0, CSR_STATUS
> > +     /* Save and clear the user space access flag. */
> > +     li    s1, SR_SUM
> > +     csrrc s0, CSR_STATUS, s1
> >       REG_S s0, TASK_THREAD_SUM_RA(a3)
> >
> >       /* Save the kernel shadow call stack pointer */
> >       scs_save_current
> >       /* Restore context from next->thread */
> >       REG_L s0,  TASK_THREAD_SUM_RA(a4)
> > -     li    s1,  SR_SUM
> >       and   s0,  s0, s1
> >       csrs  CSR_STATUS, s0
> >       REG_L ra,  TASK_THREAD_RA_RA(a4)
> > --
> > 2.39.5
> >