Re: [PATCH] riscv: misaligned: disable pagefault before accessing user memory

["Clément Léger" <cleger@rivosinc.com>]

On 06/01/2025 15:37, Alexandre Ghiti wrote:
> Hi Clément,
> 
> On 03/01/2025 17:02, Clément Léger wrote:
>> Calling copy_{from/to}_user() in interrupt context might actually sleep
>> and display a BUG message:
>>
>> [   10.377019] BUG: sleeping function called from invalid context at
>> include/linux/uaccess.h:162
>> [   10.379868] in_atomic(): 0, irqs_disabled(): 1, non_block: 0, pid:
>> 88, name: ssh-keygen
>> [   10.380009] preempt_count: 0, expected: 0
>> [   10.380324] CPU: 0 UID: 0 PID: 88 Comm: ssh-keygen Not tainted
>> 6.13.0-rc5-00013-g3435cd5f1331-dirty #19
>> [   10.380639] Hardware name: riscv-virtio,qemu (DT)
>> [   10.380798] Call Trace:
>> [   10.381108] [<ffffffff80013d30>] dump_backtrace+0x1c/0x24
>> [   10.381690] [<ffffffff800022d8>] show_stack+0x28/0x34
>> [   10.381812] [<ffffffff8000ee1c>] dump_stack_lvl+0x4a/0x68
>> [   10.381958] [<ffffffff8000ee4e>] dump_stack+0x14/0x1c
>> [   10.382047] [<ffffffff80065e0a>] __might_resched+0xfa/0x104
>> [   10.382172] [<ffffffff80065e56>] __might_sleep+0x42/0x66
>> [   10.382267] [<ffffffff801b0c5e>] __might_fault+0x1c/0x24
>> [   10.382363] [<ffffffff804415e4>] _copy_from_user+0x28/0xc2
>> [   10.382459] [<ffffffff800152ca>] handle_misaligned_load+0x1ca/0x2fc
>> [   10.382565] [<ffffffff80a033a0>] do_trap_load_misaligned+0x24/0xee
>> [   10.382714] [<ffffffff80a0dc66>] handle_exception+0x146/0x152
>>
>> In order to safely handle user memory access from this context, disable
>> page fault while copying user memory. Although this might lead to copy
>> failure in some cases (offlined page), this is the best we can try to be
>> safe.
>>
>> Fixes: b686ecdeacf6 ("riscv: misaligned: Restrict user access to
>> kernel memory")
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>>
>> ---
>>   arch/riscv/kernel/traps_misaligned.c | 26 ++++++++++++++++++++++----
>>   1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/
>> traps_misaligned.c
>> index 7cc108aed74e..75a08ed20070 100644
>> --- a/arch/riscv/kernel/traps_misaligned.c
>> +++ b/arch/riscv/kernel/traps_misaligned.c
>> @@ -355,7 +355,7 @@ static int handle_scalar_misaligned_load(struct
>> pt_regs *regs)
>>   {
>>       union reg_data val;
>>       unsigned long epc = regs->epc;
>> -    unsigned long insn;
>> +    unsigned long insn, copy_len;
>>       unsigned long addr = regs->badaddr;
>>       int fp = 0, shift = 0, len = 0;
>>   @@ -441,7 +441,16 @@ static int handle_scalar_misaligned_load(struct
>> pt_regs *regs)
>>         val.data_u64 = 0;
>>       if (user_mode(regs)) {
>> -        if (copy_from_user(&val, (u8 __user *)addr, len))
>> +        /*
>> +         * We can not sleep in exception context. Disable pagefault to
>> +         * avoid a potential sleep while accessing user memory. Side
>> +         * effect is that if it would have sleep, then the copy will
>> +         * fail.
>> +         */
>> +        pagefault_disable();
>> +        copy_len = copy_from_user(&val, (u8 __user *)addr, len);
>> +        pagefault_enable();
>> +        if (copy_len)
>>               return -1;
>>       } else {
>>           memcpy(&val, (u8 *)addr, len);
>> @@ -463,7 +472,7 @@ static int handle_scalar_misaligned_store(struct
>> pt_regs *regs)
>>   {
>>       union reg_data val;
>>       unsigned long epc = regs->epc;
>> -    unsigned long insn;
>> +    unsigned long insn, copy_len;
>>       unsigned long addr = regs->badaddr;
>>       int len = 0, fp = 0;
>>   @@ -539,7 +548,16 @@ static int
>> handle_scalar_misaligned_store(struct pt_regs *regs)
>>           return -EOPNOTSUPP;
>>         if (user_mode(regs)) {
>> -        if (copy_to_user((u8 __user *)addr, &val, len))
>> +        /*
>> +         * We can not sleep in exception context. Disable pagefault to
>> +         * avoid a potential sleep while accessing user memory. Side
>> +         * effect is that if it would have sleep, then the copy will
>> +         * fail.
>> +         */
>> +        pagefault_disable();
>> +        copy_len = copy_to_user((u8 __user *)addr, &val, len);
>> +        pagefault_enable();
>> +        if (copy_len)
>>               return -1;
>>       } else {
>>           memcpy((u8 *)addr, &val, len);
> 
> 
> I'm wondering why the irqs are disabled here? What prevents us from
> enabling them? IIUC, if for some reason the user page is not in memory,
> we will fail the misaligned access and kill the process, so such
> failures could be completely random right?

Hi Alex,

This code is called from interrupt context as a result from an
exception. AFAIK, interrupts are disabled during the whole exception
handling and re-enabled when returning to user mode in
do_notify_resume(). So that's why it triggers this warning.

Clément

> 
> But if we go for this solution, I think the same needs to be done in
> __read_insn().
> 
> Let me know if I misunderstood something.
> 
> Thanks,
> 
> Alex
>