2021-07-20 08:58:45

by Akira Tsukamoto

[permalink] [raw]
Subject: [PATCH 0/4] __asm_copy_to-from_user: Fixes

These are series for the fix reported by Guenter, Geert and Qiu.

One patch to fix overrun memory access, one patch to fix on rv32.
And two more for clean up and typos.

Have tested on qemu rv32, qemu rv64 and beaglev beta board.

Thanks for the report and instructions to reproduce the error on rv32.

Akira

Akira Tsukamoto (4):
riscv: __asm_copy_to-from_user: Fix: overrun copy
riscv: __asm_copy_to-from_user: Fix: fail on RV32
riscv: __asm_copy_to-from_user: Remove unnecessary size check
riscv: __asm_copy_to-from_user: Fix: Typos in comments

arch/riscv/lib/uaccess.S | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)

--
2.17.1


2021-07-20 08:59:38

by Akira Tsukamoto

[permalink] [raw]
Subject: [PATCH 2/4] riscv: __asm_copy_to-from_user: Fix: fail on RV32


Had a bug when converting bytes to bits when the cpu was rv32.

The a3 contains the number of bytes and multiple of 8
would be the bits. The LGREG is holding 2 for RV32 and 3 for
RV32, so to achieve multiple of 8 it must always be constant 3.
The 2 was mistakenly used for rv32.

Signed-off-by: Akira Tsukamoto <[email protected]>
---
arch/riscv/lib/uaccess.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index 8bbeca89a93f..279876821969 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -125,7 +125,7 @@ ENTRY(__asm_copy_from_user)
* t3 - prev shift
* t4 - current shift
*/
- slli t3, a3, LGREG
+ slli t3, a3, 3 /* converting bytes in a3 to bits */
li a5, SZREG*8
sub t4, a5, t3

--
2.17.1


2021-07-20 09:00:09

by Akira Tsukamoto

[permalink] [raw]
Subject: [PATCH 3/4] riscv: __asm_copy_to-from_user: Remove unnecessary size check


Clean up:

The size of 0 will be evaluated in the next step. Not
required here.

Signed-off-by: Akira Tsukamoto <[email protected]>
---
arch/riscv/lib/uaccess.S | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index 279876821969..54d497a03164 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -30,7 +30,6 @@ ENTRY(__asm_copy_from_user)
* t0 - end of uncopied dst
*/
add t0, a0, a2
- bgtu a0, t0, 5f

/*
* Use byte copy only if too small.
--
2.17.1

2021-07-20 09:01:57

by Akira Tsukamoto

[permalink] [raw]
Subject: [PATCH 4/4] riscv: __asm_copy_to-from_user: Fix: Typos in comments


Fixing typos and grammar mistakes and using more intuitive label
name.

Signed-off-by: Akira Tsukamoto <[email protected]>
---
arch/riscv/lib/uaccess.S | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/lib/uaccess.S b/arch/riscv/lib/uaccess.S
index 54d497a03164..63bc691cff91 100644
--- a/arch/riscv/lib/uaccess.S
+++ b/arch/riscv/lib/uaccess.S
@@ -33,19 +33,20 @@ ENTRY(__asm_copy_from_user)

/*
* Use byte copy only if too small.
+ * SZREG holds 4 for RV32 and 8 for RV64
*/
li a3, 9*SZREG /* size must be larger than size in word_copy */
bltu a2, a3, .Lbyte_copy_tail

/*
- * Copy first bytes until dst is align to word boundary.
+ * Copy first bytes until dst is aligned to word boundary.
* a0 - start of dst
* t1 - start of aligned dst
*/
addi t1, a0, SZREG-1
andi t1, t1, ~(SZREG-1)
/* dst is already aligned, skip */
- beq a0, t1, .Lskip_first_bytes
+ beq a0, t1, .Lskip_align_dst
1:
/* a5 - one byte for copying data */
fixup lb a5, 0(a1), 10f
@@ -54,7 +55,7 @@ ENTRY(__asm_copy_from_user)
addi a0, a0, 1 /* dst */
bltu a0, t1, 1b /* t1 - start of aligned dst */

-.Lskip_first_bytes:
+.Lskip_align_dst:
/*
* Now dst is aligned.
* Use shift-copy if src is misaligned.
@@ -71,7 +72,6 @@ ENTRY(__asm_copy_from_user)
*
* a0 - start of aligned dst
* a1 - start of aligned src
- * a3 - a1 & mask:(SZREG-1)
* t0 - end of aligned dst
*/
addi t0, t0, -(8*SZREG) /* not to over run */
@@ -106,7 +106,7 @@ ENTRY(__asm_copy_from_user)
* For misaligned copy we still perform aligned word copy, but
* we need to use the value fetched from the previous iteration and
* do some shifts.
- * This is safe because reading less than a word size.
+ * This is safe because reading is less than a word size.
*
* a0 - start of aligned dst
* a1 - start of src
@@ -116,7 +116,7 @@ ENTRY(__asm_copy_from_user)
*/
/* calculating aligned word boundary for dst */
andi t1, t0, ~(SZREG-1)
- /* Converting unaligned src to aligned arc */
+ /* Converting unaligned src to aligned src */
andi a1, a1, ~(SZREG-1)

/*
@@ -128,7 +128,7 @@ ENTRY(__asm_copy_from_user)
li a5, SZREG*8
sub t4, a5, t3

- /* Load the first word to combine with seceond word */
+ /* Load the first word to combine with second word */
fixup REG_L a5, 0(a1), 10f

3:
@@ -160,7 +160,7 @@ ENTRY(__asm_copy_from_user)
* a1 - start of remaining src
* t0 - end of remaining dst
*/
- bgeu a0, t0, 5f
+ bgeu a0, t0, .Lout_copy_user /* check if end of copy */
4:
fixup lb a5, 0(a1), 10f
addi a1, a1, 1 /* src */
@@ -168,7 +168,7 @@ ENTRY(__asm_copy_from_user)
addi a0, a0, 1 /* dst */
bltu a0, t0, 4b /* t0 - end of dst */

-5:
+.Lout_copy_user:
/* Disable access to user memory */
csrc CSR_STATUS, t6
li a0, 0
--
2.17.1

2021-07-20 09:36:53

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 0/4] __asm_copy_to-from_user: Fixes

Hi Tsukamoto-san,

On Tue, Jul 20, 2021 at 10:49 AM Akira Tsukamoto
<[email protected]> wrote:
> These are series for the fix reported by Guenter, Geert and Qiu.
>
> One patch to fix overrun memory access, one patch to fix on rv32.
> And two more for clean up and typos.
>
> Have tested on qemu rv32, qemu rv64 and beaglev beta board.
>
> Thanks for the report and instructions to reproduce the error on rv32.
>
> Akira
>
> Akira Tsukamoto (4):
> riscv: __asm_copy_to-from_user: Fix: overrun copy
> riscv: __asm_copy_to-from_user: Fix: fail on RV32
> riscv: __asm_copy_to-from_user: Remove unnecessary size check
> riscv: __asm_copy_to-from_user: Fix: Typos in comments
>
> arch/riscv/lib/uaccess.S | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)

Thanks, RV32 (vexriscv) is booting fine again.

Tested-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-07-20 09:51:07

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 2/4] riscv: __asm_copy_to-from_user: Fix: fail on RV32

Hi Tsukamoto-san,

Thanks for your patch!

On Tue, Jul 20, 2021 at 10:51 AM Akira Tsukamoto
<[email protected]> wrote:
> Had a bug when converting bytes to bits when the cpu was rv32.
>
> The a3 contains the number of bytes and multiple of 8
> would be the bits. The LGREG is holding 2 for RV32 and 3 for
> RV32, so to achieve multiple of 8 it must always be constant 3.

RV64

> The 2 was mistakenly used for rv32.
>
> Signed-off-by: Akira Tsukamoto <[email protected]>

Gr{oetje,eeting}s,

Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-07-20 10:22:49

by Akira Tsukamoto

[permalink] [raw]
Subject: Re: [PATCH 2/4] riscv: __asm_copy_to-from_user: Fix: fail on RV32

Hi Geert,

On 7/20/2021 6:49 PM, Geert Uytterhoeven wrote:
> Hi Tsukamoto-san,
>
> Thanks for your patch!
>
> On Tue, Jul 20, 2021 at 10:51 AM Akira Tsukamoto
> <[email protected]> wrote:
>> Had a bug when converting bytes to bits when the cpu was rv32.
>>
>> The a3 contains the number of bytes and multiple of 8
>> would be the bits. The LGREG is holding 2 for RV32 and 3 for
>> RV32, so to achieve multiple of 8 it must always be constant 3.
>
> RV64

Thanks, the LGREG is holding 2 for RV32 and 3 for RV64 (not RV32).

Akira

2021-07-20 14:39:40

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH 0/4] __asm_copy_to-from_user: Fixes

On 7/20/21 1:49 AM, Akira Tsukamoto wrote:
> These are series for the fix reported by Guenter, Geert and Qiu.
>
> One patch to fix overrun memory access, one patch to fix on rv32.
> And two more for clean up and typos.
>
> Have tested on qemu rv32, qemu rv64 and beaglev beta board.
>
> Thanks for the report and instructions to reproduce the error on rv32.
>
> Akira
>
> Akira Tsukamoto (4):
> riscv: __asm_copy_to-from_user: Fix: overrun copy
> riscv: __asm_copy_to-from_user: Fix: fail on RV32
> riscv: __asm_copy_to-from_user: Remove unnecessary size check
> riscv: __asm_copy_to-from_user: Fix: Typos in comments
>
> arch/riscv/lib/uaccess.S | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)
>

For the series:

Tested-by: Guenter Roeck <[email protected]>

Tested with qemu for both riscv32 and riscv64.

Thanks!

Guenter

2021-07-24 01:00:53

by Palmer Dabbelt

[permalink] [raw]
Subject: Re: [PATCH 0/4] __asm_copy_to-from_user: Fixes

On Tue, 20 Jul 2021 01:49:42 PDT (-0700), [email protected] wrote:
> These are series for the fix reported by Guenter, Geert and Qiu.
>
> One patch to fix overrun memory access, one patch to fix on rv32.
> And two more for clean up and typos.
>
> Have tested on qemu rv32, qemu rv64 and beaglev beta board.
>
> Thanks for the report and instructions to reproduce the error on rv32.
>
> Akira
>
> Akira Tsukamoto (4):
> riscv: __asm_copy_to-from_user: Fix: overrun copy
> riscv: __asm_copy_to-from_user: Fix: fail on RV32
> riscv: __asm_copy_to-from_user: Remove unnecessary size check
> riscv: __asm_copy_to-from_user: Fix: Typos in comments
>
> arch/riscv/lib/uaccess.S | 27 +++++++++++++--------------
> 1 file changed, 13 insertions(+), 14 deletions(-)

Thanks, these are on fixes.