From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f169.google.com (mail-pl1-f169.google.com [209.85.214.169]) (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 CEA8E38FB9 for ; Mon, 6 Jan 2025 14:51:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.169 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736175076; cv=none; b=stsCuQ3AoN4kNrgwbZIub0vcZnFKI8I01UppFlPhcAKxFK6CnYKLnhwLjG1rzqqU9y1PqYXVe1NYlc1kQ08qYhyH+q/xB4clUB2yahMMtXzLF4UGWLY6MJv4hNqDksDhCAhEow3wTW5lk++qztI/AN9fR8h4LIO7Ft8ouuDELQE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736175076; c=relaxed/simple; bh=GavQjwBkGMOioip/xUHVdoikazUDYhBSDI7Atz4iV10=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=aGW6FQF7xCTDnThnSkLB0fkVTi5mGIAinREtV/l7DVgTY/8/I6T+UQzbnJKjjo3421+agK2UvkDLvJQKs7B2BYv377EE59M2ssaYtPa7YegBs4LScYC4WzyHzzbORwWcBf4DfHBLzlvrIsdnTeHlm6yXZQr+A/I9DB1YiBCJX6Q= 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=hkKXDwWh; arc=none smtp.client-ip=209.85.214.169 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="hkKXDwWh" Received: by mail-pl1-f169.google.com with SMTP id d9443c01a7336-21644aca3a0so25787675ad.3 for ; Mon, 06 Jan 2025 06:51:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1736175073; x=1736779873; 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=tOK73fk5BcqpXBWGXiXK/2r2Q/HSBhtDNxcil5lgxY4=; b=hkKXDwWhpqcWQ5qGsR8RXKnKgmMgZVMA7Kwht8t4CUpdu4QXxezL5ngCxZO5rWIv+P 4lpJ+ga1EAI4lHUrxTI7XaR9gFTb42EFvVugw7i29LO84WlccOKzcOvfCEdaKtG8wOEa OCcimiZ9vomsxM+OQt9r5jU821YoS5krcv4UbpAE+4gV8bBk8OhqiNHm1xZWQqRyvfu1 TKy1HxWpX5JkffM6K46JS0v3wM6WfnNXP+BhnMha16p8Nc8Y52pu4BiqmgJPc8CYohLd HYagfHoak70Wp5S6aShNpCvdERHl2WzzVymfn6oVU3es6G9FRQrdZXcNLCvy8XrDEzYD fV8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1736175073; x=1736779873; 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=tOK73fk5BcqpXBWGXiXK/2r2Q/HSBhtDNxcil5lgxY4=; b=S5+OQjLlm+ZLjquqc6HxX+1p2RYaE+RcNcnoYWfU3uGEfY/UatwSifYqwzISC2/IAr RmKYSDIOjYb8spdNYu/Z9o5/vCFHzb7+Zuy52DQBi4MhqJoGJeMMl6h89Xj0PSG+mvyq VE2C1ax5rSMf0Rk08mAB6EuMPqUKfqK+VKAfimxFu9N5OsRS96gPkufonTB1q7krHSlq J16PdwTxuMpSV2Mz29m0MRXBcDRZtAeLbnmeHxM+QDxfthpn0lJxtd1xhyQSHkXvbJAX xby5J8KGv3aEABDT0IYgBQFTdgB1EzlWGSsw/3+b0dYnBrk9bE7Gg09hdaqG0Ibbg9YU /+rA== X-Forwarded-Encrypted: i=1; AJvYcCUEffJBiYYe/OunAfNoTZbbRnU6xT4yJL+vCAC/sUjtEy0E1eqBG1h1UpNPONw1l/SsieF0GECKdhgaLbk=@vger.kernel.org X-Gm-Message-State: AOJu0Yzj7uxbtxCIb7uUqzGCWeppDt9EXdm6N6ZvoSZTmXwUH0/TfuqW 7oV3X28yKDh28lc6jIDCx93zOUsqTqFQPQvGzYluZyVrBGNpmt7J6ph4aVAlO3k= X-Gm-Gg: ASbGncv26zZpC6tg1KPJ+PJYfmKkfdXZDJy64u6BT3gTi5EQ7Ewl0vGVGKVDh+1jFzS 9baAaaUPmwlc/wP6Pal1UH+H86ztAs0qGGafATVXyMjb1Detn/UCxlvhNrLzUBZWl6EevhXabQl e5Cgs1p4oLHpzJWTyygqCC9WOplyE5LM2OTzyPOCQq5ZqgLvG6fsi0Ab9tlU+DdWSyN9a6WF7UN qfE3UKQUSHwkHu/gqQ5NZVxnHN2zqYJcSkhrDy3ixVoo+vlcJQMQL58MFvlSa4PrFfJbkNYIFMB tudgP5s3T1O5AbqRXuP5jNxWmQ== X-Google-Smtp-Source: AGHT+IEMyRp3uhokE5fpsrQ0ahliyXQ4FuvbhV4LWvfkPAe+5IdC+g7MYTMXOyD8XUASqnvoHf7qZw== X-Received: by 2002:a05:6a21:1517:b0:1e1:adb9:d1a6 with SMTP id adf61e73a8af0-1e5e08439e8mr92752061637.41.1736175073132; Mon, 06 Jan 2025 06:51:13 -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 d2e1a72fcca58-72aad830acdsm31491327b3a.46.2025.01.06.06.51.08 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 06 Jan 2025 06:51:12 -0800 (PST) Message-ID: Date: Mon, 6 Jan 2025 15:51:03 +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? > > But if we go for this solution, I think the same needs to be done in > __read_insn(). Forgot to answer to that question: Indeed, it should be modified the same way though It did not triggered any warning since in uses the "raw" access so there is no check done before accessing the memory (ie no call to might_sleep()). I'll send a V2. Thanks, Clément > > Let me know if I misunderstood something. > > Thanks, > > Alex >