linux-kernel - Re: [PATCH v3 RESEND] x86/entry: Avoid redundant CR3 write on paranoid returns

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJD7tkZzYp56d=wms7sgji1A=84Ax2Uo+=xvs7u1PtZ+mLvFhQ@mail.gmail.com>
Date: Mon, 22 Jan 2024 21:33:42 -0800
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Brendan Jackman <jackmanb@...gle.com>
Cc: luto@...nel.org, tglx@...utronix.de, peterz@...radead.org, 
	dave.hansen@...ux.intel.com, mingo@...hat.com, bp@...en8.de, x86@...nel.org, 
	hpa@...or.com, linux-kernel@...r.kernel.org, laijs@...ux.alibaba.com, 
	reijiw@...gle.com, oweisse@...gle.com
Subject: Re: [PATCH v3 RESEND] x86/entry: Avoid redundant CR3 write on
 paranoid returns

On Mon, Jan 8, 2024 at 3:39 AM Brendan Jackman <jackmanb@...gle.com> wrote:
>
> From: Lai Jiangshan <laijs@...ux.alibaba.com>
>
> This path gets used called from:
>
> 1. #NMI return.
> 2. paranoid_exit (i.e. #MCE, #VC, #DB and #DF return)
>
> Contrary to the implication in commit 21e94459110252 ("x86/mm: Optimize
> RESTORE_CR3"), the kernel never modifies CR3 in any of these exceptions,
> except for switching from user to kernel pagetables under PTI. That
> means that most of the time when returning from an exception that
> interrupted the kernel no CR3 restore is necessary. Writing CR3 is
> expensive on some machines, so this commit avoids redundant writes.
>
> I said "most of the time" because the interrupt might have come during
> kernel entry before the user->kernel CR3 switch or the during exit after
> the kernel->user switch. In the former case skipping the restore might
> actually be be fine, but definitely not the latter. So we do still need
> to check the saved CR3 and restore it if it's a user CR3.
>
> Note this code is ONLY used for returning _to kernel code_. So the only
> times where the CR3 write is necessary are in those rather special cases
> mentioned above where we are in kernel _code_ but a userspace CR3.
>
> While changing this logic the macro is given a new name to clarify its
> usage, and a comment that was describing its behaviour at the call site
> is removed.  We can also simplify the code around the SET_NOFLUSH_BIT
> invocation as we no longer need to branch to it from above.
>
> Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
> [Rewrote commit message; responded to review comments]
> Signed-off-by: Brendan Jackman <jackmanb@...gle.com>
> Change-Id: I6e56978c4753fb943a7897ff101f519514fa0827

The Change-Id line here needs to be deleted. Otherwise, it seems like
this patch keeps falling through the cracks :)

Is there anything that needs to be done here?

> ---
>
> Notes:
>     v1: https://lore.kernel.org/lkml/20230817121513.1382800-1-jackmanb@google.com/
>
>     v1->v2: Rewrote some comments, added a proper commit message, cleaned up
>         the code per tglx's suggestion.
>
>         I've kept Lai as the Author. If you prefer for the blame to
>         record the last person that touched it then that's also fine
>         though, I can credit Lai as Co-developed-by.
>
>     v2: https://lore.kernel.org/lkml/20230920150443.1789000-1-jackmanb@google.com/
>
>     v2->v3: Clarified the commit message per Dave's suggestion and renamed the
>         macro. I did not carry PeterZ's ack since I have made some changes.
>
>     original v3 (no responses):
>         https://lore.kernel.org/lkml/20231108171656.3444702-1-jackmanb@google.com/