Re: [PATCH v3] riscv: fix strnlen() overflow in Zbb implementation

[David Laight <david.laight.linux@gmail.com>]

On Wed, 30 Sep 2026 09:48:57 +0800 (CST)
<gao.rui@zte.com.cn> wrote:

> riscv: fix strnlen() overflow in Zbb implementation
> 
> The RISC-V Zbb optimized strnlen() implementation can return incorrect
> results when very large count values are supplied.
> 
> The previous implementation calculates an end address based on the
> input pointer and count. When count is close to SIZE_MAX, the address
> calculation may overflow, resulting in incorrect termination checks and
> wrong return values.
> 
> This issue was observed while running device-mapper tests:
> 
> dmsetup create testname9 --table "0 8 zero"
> cat /sys/block/dm-*/dm/name
> dmsetup remove testname9
> 
> Rework the Zbb implementation to use a word counter instead of an end
> address. This removes the dependency on end-address calculations,
> avoids overflow entirely, and simplifies the termination condition of
> the word scanning loop. The final length is calculated using the saved
> original pointer.
> 
> Performance was evaluated with string_bench_strnlen:
> 
> New Implementation Previous Implementation
> 
> len=0 : 71 ns/call 70 ns/call
> len=1 : 82 ns/call 81 ns/call
> len=7 : 83 ns/call 81 ns/call
> len=8 : 83 ns/call 81 ns/call
> len=16 : 94 ns/call 90 ns/call
> len=31 : 117 ns/call 113 ns/call
> len=64 : 175 ns/call 167 ns/call
> len=127 : 259 ns/call 258 ns/call
> len=512 : 802 ns/call 833 ns/call
> len=1024 : 1512 ns/call 1536 ns/call
> len=3173 : 4581 ns/call 4640 ns/call
> len=4096 : 6256 ns/call 6068 ns/call
> 
> Results show comparable performance to the previous implementation
> while fixing the overflow issue.
> 
> Fixes: 5ba15d419fab ("riscv: lib: add strnlen() implementation")
> Suggested-by: DavidLaight <david.laight.linux@gmail.com>
> Signed-off-by: Gao Rui <gao.rui@zte.com.cn>
> 
> ---
> v3:
> - Keep the original register allocation (t0-t4) and numeric labels.
> - Keep the original comments.
> - Calculate word count as (remaining - 1) / SZREG + 1, removing the
>   branch from the calculation.
> v2:
> - Rework the Zbb implementation to eliminate end-address overflow
>   instead of falling back to the generic path.
> - Use a decrementing word counter for word scanning.
> - Run KUnit string tests successfully.
> - Add string_bench_strnlen benchmark results.
> ---
>  arch/riscv/lib/strnlen.S | 54 +++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/riscv/lib/strnlen.S b/arch/riscv/lib/strnlen.S
> index a8911605c248..000000000000 100644
> --- a/arch/riscv/lib/strnlen.S
> +++ b/arch/riscv/lib/strnlen.S
> @@ -73,6 +73,9 @@
>  	/* If maxlen is 0, return 0. */
>  	beqz	a1, 3f
> 
> +	/* Save original pointer for final length calculation. */
> +	mv	t4, a0
> +
>  	/* Number of irrelevant bytes in the first word. */
>  	andi	t2, a0, SZREG-1
> 
> @@ -82,15 +85,6 @@
>  	li	t3, SZREG
>  	sub	t3, t3, t2
>  	slli	t2, t2, 3
> -
> -	/*
> -	 * Aligned boundary.  Use the address of the last valid byte
> -	 * (s + count - 1) to avoid loading a word past the count
> -	 * boundary in the loop below.  count == 0 is handled above.
> -	 */
> -	add	t4, a0, a1
> -	addi	t4, t4, -1
> -	andi	t4, t4, -SZREG
> 
>  	/* Get the first word.  */
>  	REG_L	t1, 0(t0)
> @@ -125,15 +119,26 @@
> 
>  	bgtu	t3, a0, 2f
> 
> -	/* All remaining bytes are in the first word, no loop needed. */
> -	bgeu	t0, t4, 2f
> +	/*
> +	 * Use a word counter to avoid calculating an end address,
> +	 * which might overflow when count is close to SIZE_MAX.
> +	 */
> +	sub	t2, a1, t3
> +	beqz	t2, 3f

Is that right?
The equality check seems wrong.
Does it work when there are more characters in the first word than
the caller specified length.

> 
> -	/* Prepare for the word comparison loop. */
> -	addi	t2, t0, SZREG
> +	/* Calculate the word count: count = (remaining - 1) / SZREG + 1 */
> +	addi	t2, t2, -1
> +#if defined(CONFIG_64BIT)
> +	srli	t1, t2, 3
> +#else
> +	srli	t1, t2, 2
> +#endif
> +	addi	t2, t1, 1
> +
>  	li	t3, -1
> 
>  	/*
> -	 * Our critical loop is 4 instructions and processes data in
> +	 * Our critical loop is 6 instructions and processes data in

The diff below adds one instruction - I make that 5.
But the instruction count isn't what really matters, what matters
is the number of clocks it takes the loop to execute.

>  	 * 4 byte or 8 byte chunks.
>  	 */
>  	.p2align 3
> @@ -141,23 +146,22 @@
>  	REG_L	t1, SZREG(t0)
>  	addi	t0, t0, SZREG
>  	orc.b	t1, t1
> -	bgeu	t0, t4, 4f

Hasn't that (the old code) just read beyond the end of the buffer

> -	beq	t1, t3, 1b
> +	bne	t1, t3, 4f
> +	addi	t2, t2, -1
> +	bnez	t2, 1b

You want to put the 'addi t2, t2, -1' higher up next to the other addi.
Then simple cpu will execute it while waiting for the read to complete.
The old code did that - but got it wrong.
You can do the length check while the cpu is (otherwise) stalled waiting
for the read. So something like:
1:	REG_L	t1, SZREG(t0)
	addi	t0, t0, SZREG
	addi	t2, t2, -1
	bnez	t2, 3f
	orc.b	t1, t1
	beq	t1, t3, 1b
2:	// Found a zero
	...
	ret

3:	orc.b	t1, t1
	bne	t1, t3, 2b
	// No zero

The count (t1) would need to be one lower.

Did you try just subtracting SZREG from the length-left
and doing an unsigned compare against the original size?
I think it should work and saves the setup code.

David

> +
> +	/* Scanned all words without finding NUL, return maxlen. */
> +	mv	a0, a1
> +	ret
> +
>  4:
>  	not	t1, t1
>  	CZ	t1, t1
>  	srli	t1, t1, 3
> 
> -	/* Get number of processed bytes. */
> -	sub	t2, t0, t2
> -
> -	/* Add number of characters in the first word.  */
> -	add	a0, a0, t2
> -
> -	/* Add number of characters in the last word.  */
> +	/* Length = (current - original) + offset in final word. */
> +	sub	a0, t0, t4
>  	add	a0, a0, t1
> -
> -	/* Ensure the final result does not exceed maxlen. */
>  	minu	a0, a0, a1
>  2:
>  	ret
> --
> 2.27.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv