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 <[email protected]>
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/[email protected]
> 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/[email protected]?part=5
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.