Re: [PATCH] riscv: Fix kernel crash due to PR_SET_TAGGED_ADDR_CTRL

[Samuel Holland <samuel.holland@sifive.com>]

On 2025-05-06 11:31 AM, Alexandre Ghiti wrote:
> On 05/05/2025 21:27, Alexandre Ghiti wrote:
>> On 05/05/2025 18:07, Nam Cao wrote:
>>> Hi Alex,
>>>
>>> On Mon, May 05, 2025 at 06:02:26PM +0200, Alexandre Ghiti wrote:
>>>> On 04/05/2025 12:19, Nam Cao wrote:
>>>>> When userspace does PR_SET_TAGGED_ADDR_CTRL, but Supm extension is not
>>>>> available, the kernel crashes:
>>>>>
>>>>> Oops - illegal instruction [#1]
>>>>>       [snip]
>>>>> epc : set_tagged_addr_ctrl+0x112/0x15a
>>>>>    ra : set_tagged_addr_ctrl+0x74/0x15a
>>>>> epc : ffffffff80011ace ra : ffffffff80011a30 sp : ffffffc60039be10
>>>>>       [snip]
>>>>> status: 0000000200000120 badaddr: 0000000010a79073 cause: 0000000000000002
>>>>>       set_tagged_addr_ctrl+0x112/0x15a
>>>>>       __riscv_sys_prctl+0x352/0x73c
>>>>>       do_trap_ecall_u+0x17c/0x20c
>>>>>       andle_exception+0x150/0x15c
>>>>
>>>> It seems like the csr write is triggering this illegal instruction, can you
>>>> confirm it is?
>>> Yes, it is the "csr_write(CSR_ENVCFG, envcfg);" in envcfg_update_bits().
>>>
>>>> If so, I can't find in the specification that an implementation should do
>>>> that when writing envcfg and I can't reproduce it on qemu. Where did you
>>>> see this oops?
>>> I can't find it in the spec either. I think it is up to the implementation.
>>
>>
>> The reserved fields of senvcfg are WPRI and contrary to WLRL, it does not
>> explicitly "permit" to raise an illegal instruction so I'd say it is not up to
>> the implementation, I'll ask around.
> 
> 
> So I had confirmation that WPRI should not raise an illegal instruction so
> that's an issue with the platform. Your patch is not wrong but I'd rather have
> an explicit errata, what do you think?

There is no erratum here. Allwinner D1 / T-HEAD C906 implements Ss1p11, which
was before senvcfg was added to the privileged architecture, and does not
implement any of the extensions which would imply senvcfg's existence, so the
CSR is reserved. We could check for Xlinuxenvcfg to determine if the CSR access
will raise an exception, but that does not gain anything over checking for Supm
specifically. So the fix is correct.

Reviewed-by: Samuel Holland <samuel.holland@sifive.com>

That said, I wonder if set_tagged_addr_ctrl(task, 0) should succeed when Supm is
not implemented, matching get_tagged_addr_ctrl(). Without Supm, we know that
have_user_pmlen_7 and have_user_pmlen_16 will both be false, so pmlen == 0 is
the only case where we would call envcfg_update_bits(). And we know it would be
a no-op. So an alternative fix would be to return 0 below the pmlen checks:

diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index 7c244de77180..536da9aa690e 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -309,6 +309,9 @@ long set_tagged_addr_ctrl(struct task_struct *task, unsigned
long arg)
 	if (!(arg & PR_TAGGED_ADDR_ENABLE))
 		pmlen = PMLEN_0;

+	if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM))
+		return 0;
+
 	if (mmap_write_lock_killable(mm))
 		return -EINTR;


But I don't know if this better matches what userspace would expect.

Regards,
Samuel

>>> I got this crash on the MangoPI board:
>>> https://mangopi.org/mqpro
>>>
>>> Best regards,
>>> Nam


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