[v1, 0/2] Initialize Vector registers in the first-use trap

[v1, 0/2] Initialize Vector registers in the first-use trap

[Andy Chiu]

Before applying this series, We only initialize the space for saving
Vector registers. This is not enough as Vector registers themselves also
neeeded to be initialized before dropping back into userspace. Or, we
may risk leaking a process's data left in Vector register to another
process. This can be verified by only applying the second patch and
running the test.

Andy Chiu (2):
  riscv: vector: clear V-reg in the first-use trap
  selftests: Test RISC-V Vector's first-use handler

 arch/riscv/kernel/vector.c                    |  1 +
 .../testing/selftests/riscv/vector/.gitignore |  1 +
 tools/testing/selftests/riscv/vector/Makefile |  6 +-
 .../selftests/riscv/vector/v_initval_nolibc.c | 68 +++++++++++++++++++
 4 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/riscv/vector/v_initval_nolibc.c

-- 
2.17.1


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

[v1, 1/2] riscv: vector: clear V-reg in the first-use trap

[Andy Chiu]

If there is no context switch happens after we enable V for a process,
then we return to user space with whatever left on the CPU's V registers
accessible to the process. The leaked data could belong to another
process's V-context saved from last context switch, impacting process's
confidentiality on the system.

To prevent this from happening, we clear V registers by restoring
zero'd V context after turining on V.

Fixes: cd054837243b ("riscv: Allocate user's vector context in the first-use trap")
Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 arch/riscv/kernel/vector.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/kernel/vector.c b/arch/riscv/kernel/vector.c
index f9c8e19ab301..8d92fb6c522c 100644
--- a/arch/riscv/kernel/vector.c
+++ b/arch/riscv/kernel/vector.c
@@ -167,6 +167,7 @@ bool riscv_v_first_use_handler(struct pt_regs *regs)
 		return true;
 	}
 	riscv_v_vstate_on(regs);
+	riscv_v_vstate_restore(current, regs);
 	return true;
 }
 
-- 
2.17.1


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

[v1, 2/2] selftests: Test RISC-V Vector's first-use handler

[Andy Chiu]

This add a test to check if the kernel zero-initializes all V registers
after the first-use trap handler returns.

If V registers are not zero-initialized, then the test should fail one
out of several runs:

```
 root@sifive-fpga:~# ./v_initval_nolibc
 # vl = 256
 not ok 1 detect stale values on v-regesters
 0 0 0 0 0 0 0 0   0 0 0 0 0 0 0 0
 0 4c 41 4e 47 3d 43 0   50 41 54 48 3d 2f 75 73
 72 2f 6c 6f 63 61 6c 2f   73 62 69 6e 3a 2f 75 73
 72 2f 6c 6f 63 61 6c 2f   62 69 6e 3a 2f 75 73 72
 ff ff 81 0 0 0 0 0   0 0 0 0 0 0 0 0
```

Otherwise, the test passes without errors each run.

Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
---
 .../testing/selftests/riscv/vector/.gitignore |  1 +
 tools/testing/selftests/riscv/vector/Makefile |  6 +-
 .../selftests/riscv/vector/v_initval_nolibc.c | 68 +++++++++++++++++++
 3 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/riscv/vector/v_initval_nolibc.c

diff --git a/tools/testing/selftests/riscv/vector/.gitignore b/tools/testing/selftests/riscv/vector/.gitignore
index 4f2b4e8a3b08..9ae7964491d5 100644
--- a/tools/testing/selftests/riscv/vector/.gitignore
+++ b/tools/testing/selftests/riscv/vector/.gitignore
@@ -1,2 +1,3 @@
 vstate_exec_nolibc
 vstate_prctl
+v_initval_nolibc
diff --git a/tools/testing/selftests/riscv/vector/Makefile b/tools/testing/selftests/riscv/vector/Makefile
index cd6e80bf995d..bfff0ff4f3be 100644
--- a/tools/testing/selftests/riscv/vector/Makefile
+++ b/tools/testing/selftests/riscv/vector/Makefile
@@ -2,7 +2,7 @@
 # Copyright (C) 2021 ARM Limited
 # Originally tools/testing/arm64/abi/Makefile
 
-TEST_GEN_PROGS := vstate_prctl
+TEST_GEN_PROGS := vstate_prctl v_initval_nolibc
 TEST_GEN_PROGS_EXTENDED := vstate_exec_nolibc
 
 include ../../lib.mk
@@ -13,3 +13,7 @@ $(OUTPUT)/vstate_prctl: vstate_prctl.c ../hwprobe/sys_hwprobe.S
 $(OUTPUT)/vstate_exec_nolibc: vstate_exec_nolibc.c
 	$(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \
 		-Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc
+
+$(OUTPUT)/v_initval_nolibc: v_initval_nolibc.c
+	$(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \
+		-Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc
diff --git a/tools/testing/selftests/riscv/vector/v_initval_nolibc.c b/tools/testing/selftests/riscv/vector/v_initval_nolibc.c
new file mode 100644
index 000000000000..66764edb0d52
--- /dev/null
+++ b/tools/testing/selftests/riscv/vector/v_initval_nolibc.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "../../kselftest.h"
+#define MAX_VSIZE	(8192 * 32)
+
+void dump(char *ptr, int size)
+{
+	int i = 0;
+
+	for (i = 0; i < size; i++) {
+		if (i != 0) {
+			if (i % 16 == 0)
+				printf("\n");
+			else if (i % 8 == 0)
+				printf("  ");
+		}
+		printf("%02x ", ptr[i]);
+	}
+	printf("\n");
+}
+
+int main(void)
+{
+	int i;
+	unsigned long vl;
+	char *datap, *tmp;
+
+	datap = malloc(MAX_VSIZE);
+	if (!datap) {
+		ksft_test_result_fail("fail to allocate memory for size = %lu\n", MAX_VSIZE);
+		exit(-1);
+	}
+
+	tmp = datap;
+	asm volatile (
+		".option push\n\t"
+		".option arch, +v\n\t"
+		"vsetvli	%0, x0, e8, m8, ta, ma\n\t"
+		"vse8.v		v0, (%2)\n\t"
+		"add		%1, %2, %0\n\t"
+		"vse8.v		v8, (%1)\n\t"
+		"add		%1, %1, %0\n\t"
+		"vse8.v		v16, (%1)\n\t"
+		"add		%1, %1, %0\n\t"
+		"vse8.v		v24, (%1)\n\t"
+		".option pop\n\t"
+		: "=&r" (vl), "=r" (tmp) : "r" (datap) : "memory");
+
+	ksft_print_msg("vl = %lu\n", vl);
+
+	if (datap[0] != 0x00 && datap[0] != 0xff) {
+		ksft_test_result_fail("v-regesters are not properly initialized\n");
+		dump(datap, vl * 4);
+		exit(-1);
+	}
+
+	for (i = 1; i < vl * 4; i++) {
+		if (datap[i] != datap[0]) {
+			ksft_test_result_fail("detect stale values on v-regesters\n");
+			dump(datap, vl * 4);
+			exit(-2);
+		}
+	}
+
+	free(datap);
+	ksft_exit_pass();
+	return 0;
+}
-- 
2.17.1


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

Re: [v1, 1/2] riscv: vector: clear V-reg in the first-use trap

[Björn Töpel]

Andy Chiu <andy.chiu@sifive.com> writes:

> If there is no context switch happens after we enable V for a process,
> then we return to user space with whatever left on the CPU's V registers
> accessible to the process. The leaked data could belong to another
> process's V-context saved from last context switch, impacting process's
> confidentiality on the system.
>
> To prevent this from happening, we clear V registers by restoring
> zero'd V context after turining on V.
>
> Fixes: cd054837243b ("riscv: Allocate user's vector context in the first-use trap")
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

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

Re: [v1, 2/2] selftests: Test RISC-V Vector's first-use handler

[Björn Töpel]

Andy Chiu <andy.chiu@sifive.com> writes:

> This add a test to check if the kernel zero-initializes all V registers
> after the first-use trap handler returns.
>
> If V registers are not zero-initialized, then the test should fail one
> out of several runs:
>
> ```
>  root@sifive-fpga:~# ./v_initval_nolibc
>  # vl = 256
>  not ok 1 detect stale values on v-regesters
>  0 0 0 0 0 0 0 0   0 0 0 0 0 0 0 0
>  0 4c 41 4e 47 3d 43 0   50 41 54 48 3d 2f 75 73
>  72 2f 6c 6f 63 61 6c 2f   73 62 69 6e 3a 2f 75 73
>  72 2f 6c 6f 63 61 6c 2f   62 69 6e 3a 2f 75 73 72
>  ff ff 81 0 0 0 0 0   0 0 0 0 0 0 0 0
> ```
>
> Otherwise, the test passes without errors each run.
>
> Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> ---
>  .../testing/selftests/riscv/vector/.gitignore |  1 +
>  tools/testing/selftests/riscv/vector/Makefile |  6 +-
>  .../selftests/riscv/vector/v_initval_nolibc.c | 68 +++++++++++++++++++
>  3 files changed, 74 insertions(+), 1 deletion(-)
>  create mode 100644 tools/testing/selftests/riscv/vector/v_initval_nolibc.c
>
> diff --git a/tools/testing/selftests/riscv/vector/.gitignore b/tools/testing/selftests/riscv/vector/.gitignore
> index 4f2b4e8a3b08..9ae7964491d5 100644
> --- a/tools/testing/selftests/riscv/vector/.gitignore
> +++ b/tools/testing/selftests/riscv/vector/.gitignore
> @@ -1,2 +1,3 @@
>  vstate_exec_nolibc
>  vstate_prctl
> +v_initval_nolibc
> diff --git a/tools/testing/selftests/riscv/vector/Makefile b/tools/testing/selftests/riscv/vector/Makefile
> index cd6e80bf995d..bfff0ff4f3be 100644
> --- a/tools/testing/selftests/riscv/vector/Makefile
> +++ b/tools/testing/selftests/riscv/vector/Makefile
> @@ -2,7 +2,7 @@
>  # Copyright (C) 2021 ARM Limited
>  # Originally tools/testing/arm64/abi/Makefile
>  
> -TEST_GEN_PROGS := vstate_prctl
> +TEST_GEN_PROGS := vstate_prctl v_initval_nolibc
>  TEST_GEN_PROGS_EXTENDED := vstate_exec_nolibc
>  
>  include ../../lib.mk
> @@ -13,3 +13,7 @@ $(OUTPUT)/vstate_prctl: vstate_prctl.c ../hwprobe/sys_hwprobe.S
>  $(OUTPUT)/vstate_exec_nolibc: vstate_exec_nolibc.c
>  	$(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \
>  		-Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc
> +
> +$(OUTPUT)/v_initval_nolibc: v_initval_nolibc.c
> +	$(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \
> +		-Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc

Hmm, does this build with clang? (No, bigge on my end, and can be fixed
later.)

> diff --git a/tools/testing/selftests/riscv/vector/v_initval_nolibc.c b/tools/testing/selftests/riscv/vector/v_initval_nolibc.c
> new file mode 100644
> index 000000000000..66764edb0d52
> --- /dev/null
> +++ b/tools/testing/selftests/riscv/vector/v_initval_nolibc.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "../../kselftest.h"
> +#define MAX_VSIZE	(8192 * 32)
> +
> +void dump(char *ptr, int size)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < size; i++) {
> +		if (i != 0) {
> +			if (i % 16 == 0)
> +				printf("\n");
> +			else if (i % 8 == 0)
> +				printf("  ");
> +		}
> +		printf("%02x ", ptr[i]);
> +	}
> +	printf("\n");
> +}
> +
> +int main(void)
> +{
> +	int i;
> +	unsigned long vl;
> +	char *datap, *tmp;
> +
> +	datap = malloc(MAX_VSIZE);
> +	if (!datap) {
> +		ksft_test_result_fail("fail to allocate memory for size = %lu\n", MAX_VSIZE);
> +		exit(-1);
> +	}
> +
> +	tmp = datap;
> +	asm volatile (
> +		".option push\n\t"
> +		".option arch, +v\n\t"
> +		"vsetvli	%0, x0, e8, m8, ta, ma\n\t"
> +		"vse8.v		v0, (%2)\n\t"
> +		"add		%1, %2, %0\n\t"
> +		"vse8.v		v8, (%1)\n\t"
> +		"add		%1, %1, %0\n\t"
> +		"vse8.v		v16, (%1)\n\t"
> +		"add		%1, %1, %0\n\t"
> +		"vse8.v		v24, (%1)\n\t"
> +		".option pop\n\t"
> +		: "=&r" (vl), "=r" (tmp) : "r" (datap) : "memory");
> +
> +	ksft_print_msg("vl = %lu\n", vl);
> +
> +	if (datap[0] != 0x00 && datap[0] != 0xff) {
> +		ksft_test_result_fail("v-regesters are not properly initialized\n");

Nit: "v-registers"

> +		dump(datap, vl * 4);
> +		exit(-1);
> +	}
> +
> +	for (i = 1; i < vl * 4; i++) {
> +		if (datap[i] != datap[0]) {
> +			ksft_test_result_fail("detect stale values on v-regesters\n");

Nit (dito): "v-registers", and maybe "detected".


With, or without the changes above,
Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Björn

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

Re: [v1, 2/2] selftests: Test RISC-V Vector's first-use handler

[Andy Chiu]

On Tue, Jun 27, 2023 at 3:46 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> Andy Chiu <andy.chiu@sifive.com> writes:
>
> > This add a test to check if the kernel zero-initializes all V registers
> > after the first-use trap handler returns.
> >
> > If V registers are not zero-initialized, then the test should fail one
> > out of several runs:
> >
> > ```
> >  root@sifive-fpga:~# ./v_initval_nolibc
> >  # vl = 256
> >  not ok 1 detect stale values on v-regesters
> >  0 0 0 0 0 0 0 0   0 0 0 0 0 0 0 0
> >  0 4c 41 4e 47 3d 43 0   50 41 54 48 3d 2f 75 73
> >  72 2f 6c 6f 63 61 6c 2f   73 62 69 6e 3a 2f 75 73
> >  72 2f 6c 6f 63 61 6c 2f   62 69 6e 3a 2f 75 73 72
> >  ff ff 81 0 0 0 0 0   0 0 0 0 0 0 0 0
> > ```
> >
> > Otherwise, the test passes without errors each run.
> >
> > Signed-off-by: Andy Chiu <andy.chiu@sifive.com>
> > ---
> >  .../testing/selftests/riscv/vector/.gitignore |  1 +
> >  tools/testing/selftests/riscv/vector/Makefile |  6 +-
> >  .../selftests/riscv/vector/v_initval_nolibc.c | 68 +++++++++++++++++++
> >  3 files changed, 74 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/testing/selftests/riscv/vector/v_initval_nolibc.c
> >
> > diff --git a/tools/testing/selftests/riscv/vector/.gitignore b/tools/testing/selftests/riscv/vector/.gitignore
> > index 4f2b4e8a3b08..9ae7964491d5 100644
> > --- a/tools/testing/selftests/riscv/vector/.gitignore
> > +++ b/tools/testing/selftests/riscv/vector/.gitignore
> > @@ -1,2 +1,3 @@
> >  vstate_exec_nolibc
> >  vstate_prctl
> > +v_initval_nolibc
> > diff --git a/tools/testing/selftests/riscv/vector/Makefile b/tools/testing/selftests/riscv/vector/Makefile
> > index cd6e80bf995d..bfff0ff4f3be 100644
> > --- a/tools/testing/selftests/riscv/vector/Makefile
> > +++ b/tools/testing/selftests/riscv/vector/Makefile
> > @@ -2,7 +2,7 @@
> >  # Copyright (C) 2021 ARM Limited
> >  # Originally tools/testing/arm64/abi/Makefile
> >
> > -TEST_GEN_PROGS := vstate_prctl
> > +TEST_GEN_PROGS := vstate_prctl v_initval_nolibc
> >  TEST_GEN_PROGS_EXTENDED := vstate_exec_nolibc
> >
> >  include ../../lib.mk
> > @@ -13,3 +13,7 @@ $(OUTPUT)/vstate_prctl: vstate_prctl.c ../hwprobe/sys_hwprobe.S
> >  $(OUTPUT)/vstate_exec_nolibc: vstate_exec_nolibc.c
> >       $(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \
> >               -Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc
> > +
> > +$(OUTPUT)/v_initval_nolibc: v_initval_nolibc.c
> > +     $(CC) -nostdlib -static -include ../../../../include/nolibc/nolibc.h \
> > +             -Wall $(CFLAGS) $(LDFLAGS) $^ -o $@ -lgcc
>
> Hmm, does this build with clang? (No, bigge on my end, and can be fixed
> later.)

Oops, I didn't try. I will respin the series if clang runs into
problems. I am not very confident here, but do we need to gate build
these tests if CONFIG_RISCV_ISA_V is not set? If my code fits clang
well then it probably would be some toolchain dependency issues
(guessing the ".option arch" part). Kconfig should be able to resolve
this part.

>
> > diff --git a/tools/testing/selftests/riscv/vector/v_initval_nolibc.c b/tools/testing/selftests/riscv/vector/v_initval_nolibc.c
> > new file mode 100644
> > index 000000000000..66764edb0d52
> > --- /dev/null
> > +++ b/tools/testing/selftests/riscv/vector/v_initval_nolibc.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include "../../kselftest.h"
> > +#define MAX_VSIZE    (8192 * 32)
> > +
> > +void dump(char *ptr, int size)
> > +{
> > +     int i = 0;
> > +
> > +     for (i = 0; i < size; i++) {
> > +             if (i != 0) {
> > +                     if (i % 16 == 0)
> > +                             printf("\n");
> > +                     else if (i % 8 == 0)
> > +                             printf("  ");
> > +             }
> > +             printf("%02x ", ptr[i]);
> > +     }
> > +     printf("\n");
> > +}
> > +
> > +int main(void)
> > +{
> > +     int i;
> > +     unsigned long vl;
> > +     char *datap, *tmp;
> > +
> > +     datap = malloc(MAX_VSIZE);
> > +     if (!datap) {
> > +             ksft_test_result_fail("fail to allocate memory for size = %lu\n", MAX_VSIZE);
> > +             exit(-1);
> > +     }
> > +
> > +     tmp = datap;
> > +     asm volatile (
> > +             ".option push\n\t"
> > +             ".option arch, +v\n\t"
> > +             "vsetvli        %0, x0, e8, m8, ta, ma\n\t"
> > +             "vse8.v         v0, (%2)\n\t"
> > +             "add            %1, %2, %0\n\t"
> > +             "vse8.v         v8, (%1)\n\t"
> > +             "add            %1, %1, %0\n\t"
> > +             "vse8.v         v16, (%1)\n\t"
> > +             "add            %1, %1, %0\n\t"
> > +             "vse8.v         v24, (%1)\n\t"
> > +             ".option pop\n\t"
> > +             : "=&r" (vl), "=r" (tmp) : "r" (datap) : "memory");
> > +
> > +     ksft_print_msg("vl = %lu\n", vl);
> > +
> > +     if (datap[0] != 0x00 && datap[0] != 0xff) {
> > +             ksft_test_result_fail("v-regesters are not properly initialized\n");
>
> Nit: "v-registers"
>
> > +             dump(datap, vl * 4);
> > +             exit(-1);
> > +     }
> > +
> > +     for (i = 1; i < vl * 4; i++) {
> > +             if (datap[i] != datap[0]) {
> > +                     ksft_test_result_fail("detect stale values on v-regesters\n");
>
> Nit (dito): "v-registers", and maybe "detected".
>
>
> With, or without the changes above,
> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
>
> Björn

Thanks,
Andy

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

Re: [v1, 0/2] Initialize Vector registers in the first-use trap

[Palmer Dabbelt]

On Tue, 27 Jun 2023 01:55:53 +0000, Andy Chiu wrote:
> Before applying this series, We only initialize the space for saving
> Vector registers. This is not enough as Vector registers themselves also
> neeeded to be initialized before dropping back into userspace. Or, we
> may risk leaking a process's data left in Vector register to another
> process. This can be verified by only applying the second patch and
> running the test.
> 
> [...]

Applied, thanks!

[1/2] riscv: vector: clear V-reg in the first-use trap
      https://git.kernel.org/palmer/c/75b59f2a90aa
[2/2] selftests: Test RISC-V Vector's first-use handler
      https://git.kernel.org/palmer/c/5c93c4c72fbc

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>


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

Re: [v1, 0/2] Initialize Vector registers in the first-use trap

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 27 Jun 2023 01:55:53 +0000 you wrote:
> Before applying this series, We only initialize the space for saving
> Vector registers. This is not enough as Vector registers themselves also
> neeeded to be initialized before dropping back into userspace. Or, we
> may risk leaking a process's data left in Vector register to another
> process. This can be verified by only applying the second patch and
> running the test.
> 
> [...]

Here is the summary with links:
  - [v1,1/2] riscv: vector: clear V-reg in the first-use trap
    https://git.kernel.org/riscv/c/75b59f2a90aa
  - [v1,2/2] selftests: Test RISC-V Vector's first-use handler
    https://git.kernel.org/riscv/c/5c93c4c72fbc

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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