Re: [PATCH v12 06/28] riscv/mm : ensure PROT_WRITE leads to VM_READ | VM_WRITE

Deepak Gupta
2025-04-24T01:08:50Z

On Thu, Apr 10, 2025 at 12:03:44PM +0200, Radim Krčmář wrote:
>2025-03-14T14:39:25-07:00, Deepak Gupta <debug@rivosinc.com>:
>> diff --git a/arch/riscv/include/asm/mman.h b/arch/riscv/include/asm/mman.h
>> +static inline unsigned long arch_calc_vm_prot_bits(unsigned long prot,
>> + unsigned long pkey __always_unused)
>> +{
>> + unsigned long ret = 0;
>> +
>> + /*
>> + * If PROT_WRITE was specified, force it to VM_READ | VM_WRITE.
>> + * Only VM_WRITE means shadow stack.
>> + */
>
>This function also changes PROT_WX to VM_RWX, which is effectively not
>changing anything, but I think it deserves an explicit intent.
>(At least in the commit message.)

Will do that.

>
>> + if (prot & PROT_WRITE)
>> + ret = (VM_READ | VM_WRITE);
>> + return ret;
>> +}
>> diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
>> @@ -16,6 +17,15 @@ static long riscv_sys_mmap(unsigned long addr, unsigned long len,
>> + /*
>> + * If PROT_WRITE is specified then extend that to PROT_READ
>> + * protection_map[VM_WRITE] is now going to select shadow stack encodings.
>> + * So specifying PROT_WRITE actually should select protection_map [VM_WRITE | VM_READ]
>> + * If user wants to create shadow stack then they should use `map_shadow_stack` syscall.
>> + */
>> + if (unlikely((prot & PROT_WRITE) && !(prot & PROT_READ)))
>> + prot |= PROT_READ;
>
>Why isn't the previous hunk be enough? (Or why don't we do just this?)
>
>riscv_sys_mmap() eventually calls arch_calc_vm_prot_bits(), so I'd
>rather fix each code path just once.

You're right. Above hunk (arch/riscv/include/asm/mman.h) alone should be enough.
I did this change in `sys_riscv.c` out of caution. If it feels like un-necessary,
I'll remove it. No hard feelings either way.

>
>Thanks.