mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v3] riscv: fix strnlen() overflow in Zbb implementation
@ 2026-09-30  1:48 gao.rui
  2026-09-30  8:45 ` David Laight
  0 siblings, 1 reply; 4+ messages in thread
From: gao.rui @ 2026-09-30  1:48 UTC (permalink / raw)
  To: pjw, palmer, aou, alex, jiangfeng, david.laight.linux
  Cc: linux-riscv, linux-kernel, jmontleo

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

-	/* 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
 	 * 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
-	beq	t1, t3, 1b
+	bne	t1, t3, 4f
+	addi	t2, t2, -1
+	bnez	t2, 1b
+
+	/* 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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] riscv: fix strnlen() overflow in Zbb implementation
@ 2026-09-30 12:44 gao.rui
  0 siblings, 0 replies; 4+ messages in thread
From: gao.rui @ 2026-09-30 12:44 UTC (permalink / raw)
  To: david.laight.linux
  Cc: pjw, palmer, aou, alex, jiangfeng, linux-riscv, linux-kernel, jmontleo

Hi David,

Thanks for the detailed review.

>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.
>
It does. The new code is only reached when the preceding

bgtu t3, a0, 2f

does not branch. a0 is min(offset of the first NUL in the shifted
word, a1), and the shifted first word has exactly t3 significant
bytes, so the offset is within [0, t3].

>> 
>> -    /* 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.
>

Correct, I'll fix the comment to say 5.

>>       * 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
>
Yes. The final iteration loads the aligned word containing
s + count - 1, which may extend up to SZREG - 1 bytes beyond the
logical end. That is safe because the load is SZREG-aligned and
cannot cross a page boundary, which is the same approach used in
strlen.S. The new word-count loop preserves that property.

>> -    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.
>
Done. I've moved the decrement closer to the load while keeping the
NUL check before the counter check 

>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
>
I think that approach needs some additional care around the same
overflow corner case that this patch is addressing.

For example, when a1 = SIZE_MAX and t3 = 1, the remaining-byte
counter starts at SIZE_MAX - 1. On the final iteration, subtracting
SZREG causes it to wrap into the range
[2^XLEN - SZREG + 1, 2^XLEN - 1]. Depending on the exact unsigned
termination test, that wrapped value may still compare as in-range,
making it easy to miss the termination condition.

The current implementation computes

words = (remaining - 1) / SZREG + 1

once during setup and then uses an exact decrementing word counter.
This avoids any dependence on wrapped byte counts, works for all
values of a1, and only costs a few setup instructions, so I'd
prefer to keep this approach.

 Thanks,
Rui

>> +
>> +    /* 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

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-09-30 12:44 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-30  1:48 [PATCH v3] riscv: fix strnlen() overflow in Zbb implementation gao.rui
2026-09-30  8:45 ` David Laight
2026-09-30 12:39   ` gao.rui
2026-09-30 12:44 gao.rui

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®