Re: [PATCH] perf riscv: Add SDT argument parsing for RISC-V

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Medium] The 'zero' register is translated to '%zero' for uprobes, causing the kernel to reject the probe with -EINVAL.
- [Medium] Incorrect error checking of `sdt_init_op_regex()` leads to a Use-After-Free or a crash due to uninitialized `regex_t` structures if `regcomp()` fails.
--

commit 8e1a4d45321e1b5de5891b9276cd561935f4b98f
Author: Chen Pei <cp0613@linux.alibaba.com>

perf riscv: Add SDT argument parsing for RISC-V

This commit implements __perf_sdt_arg_parse_op_riscv() to parse RISC-V SDT
probe operands and convert them into uprobe-compatible formats. It uses
regular expressions to match register and memory operand formats.

> diff --git a/tools/perf/util/perf-regs-arch/perf_regs_riscv.c b/tools/perf/util/perf-regs-arch/perf_regs_riscv.c
> index 5b5f21fcba8c6..51b43e68e0e3d 100644
> --- a/tools/perf/util/perf-regs-arch/perf_regs_riscv.c
> +++ b/tools/perf/util/perf-regs-arch/perf_regs_riscv.c

[ ... ]

> +/* RISC-V register ABI names: zero, ra, sp, gp, tp, t0-t6, s0-s11, a0-a7 */
> +#define SDT_OP_REGEX1  "^(zero|ra|sp|gp|tp|t[0-6]|s[0-9]|s1[01]|a[0-7])$"
> +
> +/* RISC-V memory operand: [-]NUM(REG) */
> +#define SDT_OP_REGEX2  "^(\\-)?([0-9]+)\\((zero|ra|sp|gp|tp|t[0-6]|s[0-9]|s1[01]|a[0-7])\\)$"

Since the 'zero' register is hardwired and not saved in pt_regs, will the
kernel reject probes with a BAD_REG_NAME error when perf translates it
to '%zero' for tracefs?

Should 'zero' be excluded from these regexes and instead treated as an
unsupported constant-like argument to be skipped with SDT_ARG_SKIP?

[ ... ]

> +static int sdt_init_op_regex(void)
> +{
> +	static int initialized;
> +	int ret = 0;
> +
> +	if (initialized)
> +		return 0;
> +
> +	ret = regcomp(&sdt_op_regex1, SDT_OP_REGEX1, REG_EXTENDED);
> +	if (ret)
> +		goto error;
> +
> +	ret = regcomp(&sdt_op_regex2, SDT_OP_REGEX2, REG_EXTENDED);
> +	if (ret)
> +		goto free_regex1;
> +
> +	initialized = 1;
> +	return 0;
> +
> +free_regex1:
> +	regfree(&sdt_op_regex1);
> +error:
> +	pr_debug4("Regex compilation error.\n");
> +	return ret;
> +}

[ ... ]

> +int __perf_sdt_arg_parse_op_riscv(char *old_op, char **new_op)
> +{
> +	int ret, new_len;
> +	regmatch_t rm[4];
> +	char prefix;

[ ... ]

> +	ret = sdt_init_op_regex();
> +	if (ret < 0)
> +		return ret;

Does this properly catch regex compilation failures? 

POSIX regcomp() returns a strictly positive integer (like REG_ESPACE)
on error, so this condition will evaluate to false if regcomp() fails.

If the first regcomp() in sdt_init_op_regex() fails, could this lead to a
crash when calling regexec() on the zero-initialized sdt_op_regex1 below?

Alternatively, if the second regcomp() fails, sdt_init_op_regex()
calls regfree(&sdt_op_regex1) before returning the positive error code.

Could this cause a use-after-free when regexec() operates on the freed
regex object?

> +
> +	if (!regexec(&sdt_op_regex1, old_op, 2, rm, 0)) {
> +		/* REG --> %REG */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260414124655.129463-1-cp0613@linux.alibaba.com?part=1