| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAfSe-sZLKCGdgHZjmAbwqsv0bUUkBcWeeXarMp7htRLCtArsA@mail.gmail.com>
Date: Wed, 11 Jun 2025 11:03:37 +0800
From: Chunyan Zhang <zhang.lyra@...il.com>
To: Palmer Dabbelt <palmer@...belt.com>
Cc: zhangchunyan@...as.ac.cn, Paul Walmsley <paul.walmsley@...ive.com>,
aou@...s.berkeley.edu, Alexandre Ghiti <alex@...ti.fr>,
Charlie Jenkins <charlie@...osinc.com>, song@...nel.org, yukuai3@...wei.com,
linux-riscv@...ts.infradead.org, linux-raid@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] raid6: riscv: Fix NULL pointer dereference issue
Hi Palmer,
On Wed, 11 Jun 2025 at 06:00, Palmer Dabbelt <palmer@...belt.com> wrote:
>
> On Tue, 10 Jun 2025 03:12:32 PDT (-0700), zhangchunyan@...as.ac.cn wrote:
> > When running the raid6 user-space test program on RISC-V QEMU, there's a
> > segmentation fault which seems caused by accessing a NULL pointer,
> > which is the pointer variable p/q in raid6_rvv*_gen/xor_syndrome_real(),
> > p/q should have been equal to dptr[x], but when I use GDB command to
> > see its value, which was 0x10 like below:
> >
> > "
> > Program received signal SIGSEGV, Segmentation fault.
> > 0x0000000000011062 in raid6_rvv2_xor_syndrome_real (disks=<optimized out>, start=0, stop=<optimized out>, bytes=4096, ptrs=<optimized out>) at rvv.c:386
> > (gdb) p p
> > $1 = (u8 *) 0x10 <error: Cannot access memory at address 0x10>
> > "
> >
> > The issue was found to be related with:
> > 1) Compile optimization
> > There's no segmentation fault if compiling the raid6test program with
> > the optimization flag -O0.
> > 2) The RISC-V vector command vsetvli
> > If not used t0 as the first parameter in vsetvli, there's no
> > segmentation fault either.
> >
> > This patch selects the 2nd solution to fix the issue.
>
> This code is super fragile, it's got a bunch of vector asm blocks in
> there that aren't declaring their cobbers. At a bare minimum we should
> have something like
>
> diff --git a/lib/raid6/rvv.c b/lib/raid6/rvv.c
> index 99dfa16d37c7..3c9b3fd9f2ed 100644
> --- a/lib/raid6/rvv.c
> +++ b/lib/raid6/rvv.c
> @@ -17,6 +17,10 @@
> #define NSIZE 16
> #endif
>
> +#ifdef __riscv_vector
> +#error "This code must be built without compiler support for vector"
> +#endif
> +
Ok, I will add this.
Thanks,
Chunyan
> static void raid6_rvv1_gen_syndrome_real(int disks, unsigned long bytes, void **ptrs)
> {
> u8 **dptr = (u8 **)ptrs;
>
> because it just won't work when built with a compiler that can use
> vector instructions.
>
> > Fixes: 6093faaf9593 ("raid6: Add RISC-V SIMD syndrome and recovery calculations")
> > Signed-off-by: Chunyan Zhang <zhangchunyan@...as.ac.cn>
> > ---
> > lib/raid6/rvv.c | 48 ++++++++++++++++++++++++++++--------------------
> > 1 file changed, 28 insertions(+), 20 deletions(-)
> >
> > diff --git a/lib/raid6/rvv.c b/lib/raid6/rvv.c
> > index bf7d5cd659e0..b193ea176d5d 100644
> > --- a/lib/raid6/rvv.c
> > +++ b/lib/raid6/rvv.c
> > @@ -23,9 +23,9 @@ static int rvv_has_vector(void)
> > static void raid6_rvv1_gen_syndrome_real(int disks, unsigned long bytes, void **ptrs)
> > {
> > u8 **dptr = (u8 **)ptrs;
> > - unsigned long d;
> > - int z, z0;
> > u8 *p, *q;
> > + unsigned long vl, d;
> > + int z, z0;
> >
> > z0 = disks - 3; /* Highest data disk */
> > p = dptr[z0 + 1]; /* XOR parity */
> > @@ -33,8 +33,9 @@ static void raid6_rvv1_gen_syndrome_real(int disks, unsigned long bytes, void **
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /* v0:wp0, v1:wq0, v2:wd0/w20, v3:w10 */
> > @@ -96,7 +97,7 @@ static void raid6_rvv1_xor_syndrome_real(int disks, int start, int stop,
> > {
> > u8 **dptr = (u8 **)ptrs;
> > u8 *p, *q;
> > - unsigned long d;
> > + unsigned long vl, d;
> > int z, z0;
> >
> > z0 = stop; /* P/Q right side optimization */
> > @@ -105,8 +106,9 @@ static void raid6_rvv1_xor_syndrome_real(int disks, int start, int stop,
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /* v0:wp0, v1:wq0, v2:wd0/w20, v3:w10 */
> > @@ -192,9 +194,9 @@ static void raid6_rvv1_xor_syndrome_real(int disks, int start, int stop,
> > static void raid6_rvv2_gen_syndrome_real(int disks, unsigned long bytes, void **ptrs)
> > {
> > u8 **dptr = (u8 **)ptrs;
> > - unsigned long d;
> > - int z, z0;
> > u8 *p, *q;
> > + unsigned long vl, d;
> > + int z, z0;
> >
> > z0 = disks - 3; /* Highest data disk */
> > p = dptr[z0 + 1]; /* XOR parity */
> > @@ -202,8 +204,9 @@ static void raid6_rvv2_gen_syndrome_real(int disks, unsigned long bytes, void **
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /*
> > @@ -284,7 +287,7 @@ static void raid6_rvv2_xor_syndrome_real(int disks, int start, int stop,
> > {
> > u8 **dptr = (u8 **)ptrs;
> > u8 *p, *q;
> > - unsigned long d;
> > + unsigned long vl, d;
> > int z, z0;
> >
> > z0 = stop; /* P/Q right side optimization */
> > @@ -293,8 +296,9 @@ static void raid6_rvv2_xor_syndrome_real(int disks, int start, int stop,
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /*
> > @@ -410,9 +414,9 @@ static void raid6_rvv2_xor_syndrome_real(int disks, int start, int stop,
> > static void raid6_rvv4_gen_syndrome_real(int disks, unsigned long bytes, void **ptrs)
> > {
> > u8 **dptr = (u8 **)ptrs;
> > - unsigned long d;
> > - int z, z0;
> > u8 *p, *q;
> > + unsigned long vl, d;
> > + int z, z0;
> >
> > z0 = disks - 3; /* Highest data disk */
> > p = dptr[z0 + 1]; /* XOR parity */
> > @@ -420,8 +424,9 @@ static void raid6_rvv4_gen_syndrome_real(int disks, unsigned long bytes, void **
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /*
> > @@ -536,7 +541,7 @@ static void raid6_rvv4_xor_syndrome_real(int disks, int start, int stop,
> > {
> > u8 **dptr = (u8 **)ptrs;
> > u8 *p, *q;
> > - unsigned long d;
> > + unsigned long vl, d;
> > int z, z0;
> >
> > z0 = stop; /* P/Q right side optimization */
> > @@ -545,8 +550,9 @@ static void raid6_rvv4_xor_syndrome_real(int disks, int start, int stop,
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /*
> > @@ -718,9 +724,9 @@ static void raid6_rvv4_xor_syndrome_real(int disks, int start, int stop,
> > static void raid6_rvv8_gen_syndrome_real(int disks, unsigned long bytes, void **ptrs)
> > {
> > u8 **dptr = (u8 **)ptrs;
> > - unsigned long d;
> > - int z, z0;
> > u8 *p, *q;
> > + unsigned long vl, d;
> > + int z, z0;
> >
> > z0 = disks - 3; /* Highest data disk */
> > p = dptr[z0 + 1]; /* XOR parity */
> > @@ -728,8 +734,9 @@ static void raid6_rvv8_gen_syndrome_real(int disks, unsigned long bytes, void **
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /*
> > @@ -912,7 +919,7 @@ static void raid6_rvv8_xor_syndrome_real(int disks, int start, int stop,
> > {
> > u8 **dptr = (u8 **)ptrs;
> > u8 *p, *q;
> > - unsigned long d;
> > + unsigned long vl, d;
> > int z, z0;
> >
> > z0 = stop; /* P/Q right side optimization */
> > @@ -921,8 +928,9 @@ static void raid6_rvv8_xor_syndrome_real(int disks, int start, int stop,
> >
> > asm volatile (".option push\n"
> > ".option arch,+v\n"
> > - "vsetvli t0, x0, e8, m1, ta, ma\n"
> > + "vsetvli %0, x0, e8, m1, ta, ma\n"
> > ".option pop\n"
> > + : "=&r" (vl)
> > );
> >
> > /*
Powered by blists - more mailing lists