From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f51.google.com (mail-pj1-f51.google.com [209.85.216.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A58EF1DCB0E for ; Mon, 6 Jan 2025 14:47:49 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736174876; cv=none; b=ETO/Vs8VJ7vQDfFLL0O0qVji0MsgVd+gO9rmTmIjIYCZ3O/V4h0QEewKFtC1OtEkGYnMvrLsdKn5Tl3wySyyG0ydthRTKsnDvq2FqQh8Vdc3Y4QpDpRrMaKqHBr0EYa5KOiIUHyov2gFWBp4ynM293ekTfxXPaz8g0BOwx929j8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736174876; c=relaxed/simple; bh=++ewvg2H9pk3PrGaQtVza+mx4RwQaWELW09Me5zoL4c=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=pWktDJn/qluqjzelas5zCmbA3C2jmOYHTTnLAM6nnTbgbkvEZzRZao7d0j3wT9f93YPDEzkDEHswHo1C0nvZ1L4J+2y13vVnY1tLtxXJwYmfM1XoVpj/OFlnxatKsTmk+SclDs+2EHnypbfA/Gu1L8xNSqE9Fn/6qubEbRL6JGE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com; spf=pass smtp.mailfrom=rivosinc.com; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b=NPdD7mrh; arc=none smtp.client-ip=209.85.216.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=rivosinc.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=rivosinc-com.20230601.gappssmtp.com header.i=@rivosinc-com.20230601.gappssmtp.com header.b="NPdD7mrh" Received: by mail-pj1-f51.google.com with SMTP id 98e67ed59e1d1-2ef714374c0so17884326a91.0 for ; Mon, 06 Jan 2025 06:47:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1736174866; x=1736779666; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=N8ShvKnJKHbBbIfzU3u2fPvT77gYzjD3QzSKw1ElEXQ=; b=NPdD7mrhVLqWIi3C2/fGG7taLfEBuk2QcHed1LbnT18TnG7r72rYaWeiDaRZDfvzzP MzHappJpkr9V8RseK6z/Xvcz5IEq0ZL/IlFcK8o7dypFVBveaWg6ht41vTWwFGarDbmp wCqS9EH5HxzM1VJby8t4HQsntT6dMVGvQiBwMtoForyDJLykj5MhG1VIfYVRR3YfckiH JiPxvuOBE+vDsTaAgozmuCLLxB0Khgxf5Z+qU9mGGk0xApW59zZAMqy9gXeOUYBBObCt GtcWJ+KGbqoLo9xaU4ERsg9aCbXfhFwsAweE2/dHoNIlGEtAzuRYJum5eMA0qdfRofC9 6hBw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736174866; x=1736779666; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=N8ShvKnJKHbBbIfzU3u2fPvT77gYzjD3QzSKw1ElEXQ=; b=Qdai5nCaLjAKOaVn1Gs+EZouNKHEPDVMdkV1OH4dLgr+QY34P8HKsz/7hCFeztVNlD uYMkI3FaidMyGIoamKgTuO3XZ2eCe5a7FzXJwOCVmCNgxlYZ+lyA4XcJ3IZEDXqpGesN iYlrILjdajvrzDk+OEVtppXxKoL+GJp/3upTTDG87oWaCV0JzbglLUrWvc/CG9K6De+g qKb+gbZ3zyl6HTIv3ccCnG4maT1Im+S6hKz60JtqV9T5BwLJhYHgOT3uzkIM5QWJO5Cy Qa/KBUO/flXnkzyj3nAWIQLDGZRc7GSRiuPwxKW5R4tqxLgNJe8OuUaM89zB1iXVYNja +SmA== X-Forwarded-Encrypted: i=1; AJvYcCWHgNzh2OLv1W35a64oO/v52yaYaiS5JgnYh6LlvfnJzQile6IdEJ8Bv0xGQgKscK5MQYbV6DZKXBvSPSY=@vger.kernel.org X-Gm-Message-State: AOJu0YxkuhwWCHjI6iOB9ta7Xo+hxWZ9ijOA1YQvJjyIeRMu+x9YcRJ/ +ZpbblyGoLO9iO7PK7lbVAhEnn5uO75dpR2MJ2VpsXd981vUIbeVVTf3or/DdLQ= X-Gm-Gg: ASbGncs0wjwtyRqEHQmVZa6RupW8H267gijSIqVOSk2E8mOR1UBSOhk33J8vWWtqDYJ fXpn5G0PE/q9Ncu2EBkOWZEazAeAUZM+kUcqN3s8Cnsz6kd8jCBLEtJujmQDg/hm/bEdEeSdlz4 IplZx42JsWbJLkVhJaZM+MRvjkYgqrxDMrk5QIWnANkETsWj7eWecztiZHzD+Ajy8BwbO+g81T1 nwzbwpn3Bk0QbLE7BVC3ECb6UDIvOhPVcWkyaY2K/hFeX9y6Fr2/FqhYEazXO7ATQxumvQ7J51j AJWIOpjAKl2+hBPEl+hI7MkMiQ== X-Google-Smtp-Source: AGHT+IEfiYiuZ2bu6M0Ryc/pi0Qit0SQ26mPdyY07+eTuW7P9drMf6YNS4OhO9F8ObsWsHhzkE7yJA== X-Received: by 2002:a17:90a:f945:b0:2ef:949c:6f6b with SMTP id 98e67ed59e1d1-2f4437bf688mr95733018a91.13.1736174865896; Mon, 06 Jan 2025 06:47:45 -0800 (PST) Received: from ?IPV6:2a01:e0a:e17:9700:16d2:7456:6634:9626? ([2a01:e0a:e17:9700:16d2:7456:6634:9626]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-2f2db9ca30asm22373253a91.1.2025.01.06.06.47.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Jan 2025 06:47:45 -0800 (PST) Message-ID: Date: Mon, 6 Jan 2025 15:47:34 +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 To: Alexandre Ghiti , Paul Walmsley , Palmer Dabbelt , "open list:RISC-V ARCHITECTURE" , open list Cc: Samuel Holland References: <20250103160214.657508-1-cleger@rivosinc.com> Content-Language: en-US From: =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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] [] 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? 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 >