[PATCH] riscv: __asm_copy_to-from_user: fix out of boundary memory copy

[PATCH] riscv: __asm_copy_to-from_user: fix out of boundary memory copy

[Qiu Wenbo]

The __asm_copy_to-from_user function will copy extra bytes beyond the
boundary when two conditions hold:

1. (src - dst) & (SZREG-1) == 0
2. 8*SZREG <= size < -src & (SZREG-1) + 8*SZREG

The first condition makes the function enter the unrolled word copy code
path. And the second condition makes the function believe that there is
enough bytes to do one iteration of 8*SZREG byte copy. That is not true
since the available bytes is reduced by -src & (SZREG-1) byte to make
both src and dst aligned to SZREG.

This behavior causes serious issue with exec system call both on RV64
and RV32. The passed-in command line parameters might be changed
silently since they are copied to the new process's stack continuously.

Fixes: ca6eaaa210de ("riscv: __asm_copy_to-from_user: Optimize unaligned memory access and pipeline stall")
Signed-off-by: Qiu Wenbo <qiuwenbo@kylinos.com.cn>
---
 arch/riscv/lib/uaccess.S | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index bceb0629e440..7ab7cb96dcd9 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -36,6 +36,9 @@ ENTRY(__asm_copy_from_user)
 	 * Use byte copy only if too small.
 	 */
 	li	a3, 8*SZREG /* size must be larger than size in word_copy */
+	neg	t1, a0
+	andi	t1, t1, SZREG-1
+	add	a3, a3, t1
 	bltu	a2, a3, .Lbyte_copy_tail
 
 	/*
-- 
2.32.0


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

Re: [PATCH] riscv: __asm_copy_to-from_user: fix out of boundary memory copy

[Akira Tsukamoto]

On 7/18/2021 1:12 AM, Qiu Wenbo wrote:
> The __asm_copy_to-from_user function will copy extra bytes beyond the
> boundary when two conditions hold:
> 
> 1. (src - dst) & (SZREG-1) == 0
> 2. 8*SZREG <= size < -src & (SZREG-1) + 8*SZREG
> 
> The first condition makes the function enter the unrolled word copy code
> path. And the second condition makes the function believe that there is
> enough bytes to do one iteration of 8*SZREG byte copy. That is not true
> since the available bytes is reduced by -src & (SZREG-1) byte to make
> both src and dst aligned to SZREG.

Thanks for analyzing the bug.

>  	li	a3, 8*SZREG /* size must be larger than size in word_copy */

Changing the 8*SZREG to 9*SZREG as bellow

li	a3, 9*SZREG

would fix it but since it is going to respin the patch
I would like to add the word_copy when the size is in between 2*SZREG
9*SZREG as Palmer have mentioned.

Akira

> 
> This behavior causes serious issue with exec system call both on RV64
> and RV32. The passed-in command line parameters might be changed
> silently since they are copied to the new process's stack continuously.
> 
> Fixes: ca6eaaa210de ("riscv: __asm_copy_to-from_user: Optimize unaligned memory access and pipeline stall")
> Signed-off-by: Qiu Wenbo <qiuwenbo@kylinos.com.cn>
> ---
>  arch/riscv/lib/uaccess.S | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
> index bceb0629e440..7ab7cb96dcd9 100644
> --- a/arch/riscv/lib/uaccess.S
> +++ b/arch/riscv/lib/uaccess.S
> @@ -36,6 +36,9 @@ ENTRY(__asm_copy_from_user)
>  	 * Use byte copy only if too small.
>  	 */
>  	li	a3, 8*SZREG /* size must be larger than size in word_copy */
> +	neg	t1, a0
> +	andi	t1, t1, SZREG-1
> +	add	a3, a3, t1
>  	bltu	a2, a3, .Lbyte_copy_tail
>  
>  	/*
> 

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

Re: [PATCH] riscv: __asm_copy_to-from_user: fix out of boundary memory copy

[Akira Tsukamoto]

Hi Qiu,

On 7/18/2021 11:05 AM, Akira Tsukamoto wrote:
> 
> On 7/18/2021 1:12 AM, Qiu Wenbo wrote:
>> The __asm_copy_to-from_user function will copy extra bytes beyond the
>> boundary when two conditions hold:
>>
>> 1. (src - dst) & (SZREG-1) == 0
>> 2. 8*SZREG <= size < -src & (SZREG-1) + 8*SZREG
>>
>> The first condition makes the function enter the unrolled word copy code
>> path. And the second condition makes the function believe that there is
>> enough bytes to do one iteration of 8*SZREG byte copy. That is not true
>> since the available bytes is reduced by -src & (SZREG-1) byte to make
>> both src and dst aligned to SZREG.
> 
> Thanks for analyzing the bug.
> 
>>  	li	a3, 8*SZREG /* size must be larger than size in word_copy */
> 
> Changing the 8*SZREG to 9*SZREG as bellow
> 
> li	a3, 9*SZREG
> 
> would fix it but since it is going to respin the patch
> I would like to add the word_copy when the size is in between 2*SZREG
> 9*SZREG as Palmer have mentioned.

I created a series to fix above on the other thread.
It had to revise two places to fix the overrunning memory copy.

>> diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
>> index bceb0629e440..7ab7cb96dcd9 100644
>> --- a/arch/riscv/lib/uaccess.S
>> +++ b/arch/riscv/lib/uaccess.S
>> @@ -36,6 +36,9 @@ ENTRY(__asm_copy_from_user)
>>  	 * Use byte copy only if too small.
>>  	 */
>>  	li	a3, 8*SZREG /* size must be larger than size in word_copy */
>> +	neg	t1, a0
>> +	andi	t1, t1, SZREG-1
>> +	add	a3, a3, t1
>>  	bltu	a2, a3, .Lbyte_copy_tail

I chosen just using 9*SZREG instead of using fine grained three lines. 

Your analysis greatly helped for creating the fixes. Please let me 
know if it still has issue. 

Thank you,

Akira

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