Re: riscv: Fix SR_SUM leakage across context switches

[Andy Chiu]

On Fri, Oct 23, 2026 at 10:50:45PM +0800, yunhui cui wrote:
> 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 for pointing this out Yunhui. I've looked into the code and this
reveals a bigger problem: calling local_bh_enable() while having a live
SR_SUM is not safe. local_bh_enable() can call softirq inline, which
calls into usercopy in perf and BPF. Since the later copy assumes being
called with SR_SUM disabled, it then flips the bit and returns with1
SR_SUM silently turned off.

The unsafe_copy_{to,from}_user() with size >= 768 can take this path and
calls local_bh_enable() with SR_SUM enabled under kernel_vector_begin()

I am going to send a patch that prevent calling local_bh_enable() in
kernel_vector_begin(). the get_cpu_vector_cotext() is actually too strong
for the requirement. Otherwise, we should consider gating vectorized
ucopy from unsafe_* path.

As for this patch:

Reviewed-by: Andy Chiu <tchiu@xxxxxxxxxxxxxxx>