Re: [PATCH v7 05/22] RISC-V: Define indirect CSR access helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Macro Variable Shadowing / Hygiene vulnerability in statement expressions
- [Medium] API Asymmetry / Hardcoded Selector in Indirect CSR Access Helpers
--

commit aa0528a3e619974b7113157843897b147ef398e9
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 functions for ease of usage.

> diff --git a/arch/riscv/include/asm/csr_ind.h b/arch/riscv/include/asm/csr_ind.h
> new file mode 100644
> index 0000000000000..6fd7d44dc640a
> --- /dev/null
> +++ b/arch/riscv/include/asm/csr_ind.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Rivos Inc.
> + */
> +
> +#ifndef _ASM_RISCV_CSR_IND_H
> +#define _ASM_RISCV_CSR_IND_H
> +
> +#include <linux/irqflags.h>
> +
> +#include <asm/csr.h>
> +
> +#define csr_ind_read(iregcsr, iselbase, iseloff) ({		\
> +	unsigned long __value = 0;				\
> +	unsigned long __flags;					\
> +	local_irq_save(__flags);				\

[Severity: Medium]
Is there a risk of variable shadowing here? Since this macro evaluates caller
arguments inside the statement expression, passing an argument named __flags
or __value could silently shadow these local variables. 

For example, if a caller passes a variable named __flags as the iselbase
argument, local_irq_save() would save the IRQ state into the inner shadowed
__flags variable before iselbase is evaluated. 

Could these internal variables use more unique namespace prefixes to avoid
collisions?

> +	csr_write(CSR_ISELECT, (iselbase) + (iseloff));		\
> +	__value = csr_read(iregcsr);				\

[Severity: Medium]
Does this hardcoded use of CSR_ISELECT create an issue if callers use other
data registers? 

The macro allows specifying the data register via iregcsr, but hardcodes the
selector. If these macros are used with virtualized state registers like
CSR_VSIREG, the macro would write to the host's CSR_ISELECT but then read from
the guest's CSR_VSIREG using an unmodified CSR_VSISELECT.

Would it be safer to either accept the selector register as an explicit
parameter or enforce that iregcsr must be the corresponding data register?

> +	local_irq_restore(__flags);				\
> +	__value;						\
> +})
> +
> +#define csr_ind_write(iregcsr, iselbase, iseloff, value) ({	\
> +	unsigned long __flags;					\
> +	local_irq_save(__flags);				\

[Severity: Medium]
Similarly, does the local __flags variable here risk shadowing arguments
passed as iselbase, iseloff, or value?

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

[Severity: Medium]
Does this macro have the same shadowing concerns with __old_val, __value, and
__flags colliding with the evaluated macro arguments?

> +	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;						\
> +})
> +
> +#endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622-counter_delegation-v7-0-0ba2fd34614e@meta.com?part=5