From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 79B9D182D2 for ; Mon, 6 Jan 2025 14:37:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.70.183.193 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736174236; cv=none; b=MqQ1LLa8tOHT7qnVKFSxJgQQTqYnJWSfhg1U48Iaj6+FxzPiITkG6l84F2wVvRf1izA4B5Fpk0Abmu4wmySOLWvbf50EJAwdYCTR42aC3gtM0hurMtR6LJQe5/eNfmKFOsiyAsUPk9GceUNfs13ZKsxvKnFI0+i+3Cghy0W9CY0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736174236; c=relaxed/simple; bh=tX8+0BB93oxycN1zivTqRrTVOmFOY+69SWwIhzKLkf4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=EbnLND1tqscCIHCT8erkzTmSm7e9GPfPBYXc91DXwwW5NSRpJYtmMJtT3Ue1PYe/cH2v1YFQxhHqln9ffcZvNSon3lJH6W6+wqcvKEnutUGmL43MP+Uz0axJfLQlij0BwGf5gID63pdvMJ9+4aGG+SAojucPZUwlGxuz3Q6Wsdo= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ghiti.fr; spf=pass smtp.mailfrom=ghiti.fr; arc=none smtp.client-ip=217.70.183.193 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=ghiti.fr Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=ghiti.fr Received: by mail.gandi.net (Postfix) with ESMTPSA id DB71C24000E; Mon, 6 Jan 2025 14:37:10 +0000 (UTC) Message-ID: Date: Mon, 6 Jan 2025 15:37:10 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] riscv: misaligned: disable pagefault before accessing user memory Content-Language: en-US To: =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= , Paul Walmsley , Palmer Dabbelt , "open list:RISC-V ARCHITECTURE" , open list Cc: Samuel Holland References: <20250103160214.657508-1-cleger@rivosinc.com> From: Alexandre Ghiti In-Reply-To: <20250103160214.657508-1-cleger@rivosinc.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-GND-Sasl: alex@ghiti.fr 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] [] dump_backtrace+0x1c/0x24 > [ 10.381690] [] show_stack+0x28/0x34 > [ 10.381812] [] dump_stack_lvl+0x4a/0x68 > [ 10.381958] [] dump_stack+0x14/0x1c > [ 10.382047] [] __might_resched+0xfa/0x104 > [ 10.382172] [] __might_sleep+0x42/0x66 > [ 10.382267] [] __might_fault+0x1c/0x24 > [ 10.382363] [] _copy_from_user+0x28/0xc2 > [ 10.382459] [] handle_misaligned_load+0x1ca/0x2fc > [ 10.382565] [] do_trap_load_misaligned+0x24/0xee > [ 10.382714] [] 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 > > --- > 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? 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