linux-kernel - Re: [PATCH v9 16/17] riscv: Fix an illegal instruction exception when accessing vlenb without enable vector first

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d08f105-64fd-a43a-1dea-24870ff7c5b0@codethink.co.uk>
Date:   Wed, 10 Nov 2021 10:38:20 +0000
From:   Ben Dooks <ben.dooks@...ethink.co.uk>
To:     Greentime Hu <greentime.hu@...ive.com>, palmer@...belt.com,
        paul.walmsley@...ive.com, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, aou@...s.berkeley.edu
Subject: Re: [PATCH v9 16/17] riscv: Fix an illegal instruction exception when
 accessing vlenb without enable vector first

On 09/11/2021 09:48, Greentime Hu wrote:
> It triggered an illegal instruction exception when accessing vlenb CSR
> without enable vector first. To fix this issue, we should enable vector
> before using it and disable vector after using it.
> 
> Co-developed-by: Vincent Chen <vincent.chen@...ive.com>
> Signed-off-by: Vincent Chen <vincent.chen@...ive.com>
> Signed-off-by: Greentime Hu <greentime.hu@...ive.com>
> ---
>   arch/riscv/include/asm/vector.h        | 2 ++
>   arch/riscv/kernel/cpufeature.c         | 2 ++
>   arch/riscv/kernel/kernel_mode_vector.c | 6 ++++--
>   3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 5d7f14453f68..ca063c8f47f2 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -8,6 +8,8 @@
>   
>   #include <linux/types.h>
>   
> +void rvv_enable(void);
> +void rvv_disable(void);
>   void kernel_rvv_begin(void);
>   void kernel_rvv_end(void);
>   
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 8e7557980faf..0139ec20adce 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -159,7 +159,9 @@ void __init riscv_fill_hwcap(void)
>   	if (elf_hwcap & COMPAT_HWCAP_ISA_V) {
>   		static_branch_enable(&cpu_hwcap_vector);
>   		/* There are 32 vector registers with vlenb length. */
> +		rvv_enable();
>   		riscv_vsize = csr_read(CSR_VLENB) * 32;
> +		rvv_disable();
>   	}
>   #endif

Would it be better to enable this here, and then restore the original
state instead of calling rvv_disable? If it was enabled then maybe
something else is going to rely on that state?

Maybe something like:

		prev = rvv_enable()
		riscv_vsize = csr_read(CSR_VLENB) * 32;
		rvv_restore(prev);


>   }
> diff --git a/arch/riscv/kernel/kernel_mode_vector.c b/arch/riscv/kernel/kernel_mode_vector.c
> index 8d2e53ea25c1..1ecb6ec5c56d 100644
> --- a/arch/riscv/kernel/kernel_mode_vector.c
> +++ b/arch/riscv/kernel/kernel_mode_vector.c
> @@ -71,15 +71,17 @@ static void put_cpu_vector_context(void)
>   	preempt_enable();
>   }
>   
> -static void rvv_enable(void)
> +void rvv_enable(void)
>   {
>   	csr_set(CSR_STATUS, SR_VS);
>   }
> +EXPORT_SYMBOL(rvv_enable);
>   
> -static void rvv_disable(void)
> +void rvv_disable(void)
>   {
>   	csr_clear(CSR_STATUS, SR_VS);
>   }
> +EXPORT_SYMBOL(rvv_disable);
>   
>   /*
>    * kernel_rvv_begin(): obtain the CPU vector registers for use by the calling
> 


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html