Re: [PATCH v9 05/20] RISC-V: Define indirect CSR access helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The indirect CSR access macros use generic local variable names (`__flags`, `__value`, `__old_val`) which can silently shadow caller arguments, leading to logic corruption and unintended hardware writes.
--

--- Patch [5]: [PATCH v9 05/20] RISC-V: Define indirect CSR access helpers ---
commit abb666a0789f79a51327c355784dec21386f3bf0
Author: Atish Patra <atishp@rivosinc.com>

RISC-V: Define indirect CSR access helpers

The indirect CSR requires multiple instructions to read/write CSR. Add a few
helper macros for ease of usage.

Link: https://patch.msgid.link/20260701-counter_delegation-v8-5-7909f863a645@meta.com

> diff --git a/arch/riscv/include/asm/csr_indirect.h b/arch/riscv/include/asm/csr_indirect.h
> new file mode 100644
> index 0000000000000..0f558fac8f5f7
> --- /dev/null
> +++ b/arch/riscv/include/asm/csr_indirect.h
[ ... ]
> +#define csr_indirect_read(iregcsr, iselbase, iseloff) ({		\
> +	unsigned long __value = 0;				\
> +	unsigned long __flags;					\

[Severity: Medium]
Could these generic local variable names silently shadow caller arguments?

If a caller passes an expression using __flags or __value for iselbase or
iseloff, the macro's local variables will shadow them, potentially causing
the macro to use unintended values.

> +	local_irq_save(__flags);				\
> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	__value = csr_read(iregcsr);				\
> +	local_irq_restore(__flags);				\
> +	__value;						\
> +})
> +
> +#define csr_indirect_write(iregcsr, iselbase, iseloff, value) ({	\
> +	unsigned long __flags;					\

[Severity: Medium]
Can this local __flags variable shadow the value argument if the caller
passes a variable named __flags?

If a caller executes csr_indirect_write(reg, base, off, __flags), the macro
would evaluate (value) using its own __flags variable (which holds the saved
IRQ state) instead of the caller's value. This would write the IRQ state to
the hardware CSR, potentially corrupting the CPU state.

> +	local_irq_save(__flags);				\
> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	csr_write(iregcsr, (value));				\
> +	local_irq_restore(__flags);				\
> +})
> +
> +#define csr_indirect_warl(iregcsr, iselbase, iseloff, warl_val) ({	\
> +	unsigned long __old_val = 0, __value = 0;		\
> +	unsigned long __flags;					\

[Severity: Medium]
Similar to the other macros, do these generic local variable names risk
shadowing the warl_val, iselbase, or iseloff arguments?

> +	local_irq_save(__flags);				\
> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	__old_val = csr_read(iregcsr);				\
> +	csr_write(iregcsr, (warl_val));				\
> +	__value = csr_read(iregcsr);				\
> +	csr_write(iregcsr, __old_val);				\
> +	local_irq_restore(__flags);				\
> +	__value;						\
> +})

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-counter_delegation-v9-0-58658104e487@meta.com?part=5