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: <4ce0b11c-d2fd-4dff-b9db-30e50500ee83@google.com>
Date: Mon, 17 Mar 2025 17:50:19 -0700
From: Junaid Shahid <junaids@...gle.com>
To: Brendan Jackman <jackmanb@...gle.com>, Borislav Petkov <bp@...en8.de>
Cc: akpm@...ux-foundation.org, dave.hansen@...ux.intel.com,
 yosryahmed@...gle.com, kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, peterz@...radead.org, seanjc@...gle.com,
 tglx@...utronix.de, x86@...nel.org
Subject: Re: [PATCH RFC v2 03/29] mm: asi: Introduce ASI core API

On 3/17/25 4:40 AM, Brendan Jackman wrote:
> 
> I don't understand having both asi_[un]lock() _and_
> asi_{start,enter}_critical_region(). The only reason we need the
> critical section concept is for the purposes of the NMI glue code you
> mentioned in part 1, and that setup must happen before the switch into
> the restricted address space.
> 
> Also, I don't think we want part 5 inside the asi_lock()->asi_unlock()
> region. That seems like the region betwen part 5 and 6, we are in the
> unrestricted address space, but the NMI entry code is still set up to
> return to the restricted address space on exception return. I think
> that would actually be harmless, but it doesn't achieve anything.
> 
> The more I talk about it, the more convinced I am that the proper API
> should only have two elements, one that says "I'm about to run
> untrusted code" and one that says "I've finished running untrusted
> code". But...
> 
>> 1. you can do empty calls to keep the interface balanced and easy to use
>>
>> 2. once you can remove asi_exit(), you should be able to replace all in-tree
>>     users in one atomic change so that they're all switched to the new,
>>     simplified interface
> 
> Then what about if we did this:
> 
> /*
>   * Begin a region where ASI restricted address spaces _may_ be used.
>   *
>   * Preemption must be off throughout this region.
>   */
> static inline void asi_start(void)
> {
> 	/*
> 	 * Cannot currently context switch in the restricted adddress
> 	 * space.
> 	 */
> 	lockdep_assert_preemption_disabled();

I assume that this limitation is just for the initial version in this RFC, 
right? But even in that case, I think this should be in asi_start_critical() 
below, not asi_start(), since IIRC the KVM run loop does contain preemptible 
code as well. And we would need an explicit asi_exit() in the context switch 
code like we had in an earlier RFC.

> 
> 	/*
> 	 * (Actually, this doesn't do anything besides assert, it's
> 	 * just to help the API make sense).
> 	 */
> }
> 
> /*
>   * End a region begun by asi_start(). After this, the CPU cannot be in
>   * the restricted address space until the next asi_start().
>   */
> static inline void asi_end(void)
> {
> 	/* Leave the restricted address space if we're in it. */
> 	...
> }
> 
> /*
>   * About to run untrusted code, begin a region that _must_ run in the
>   * restricted address space.
>   */
> void asi_start_critical(void);
> 
> /* End a region begun by asi_start_critical(). */
> void asi_end_critical(void);
> 
> ioctl(KVM_RUN) {
>      enter_from_user_mode()
>      asi_start()
>      while !need_userspace_handling()
>          asi_start_critical();
>          vmenter();
>          asi_end_critical();
>      }
>      asi_end()
>      exit_to_user_mode()
> }
> 
> Then the API is balanced, and we have a clear migration path towards
> the two-element API, i.e. we need to just remove asi_start() and
> asi_end(). It also better captures the point about the temporary
> simplification: basically the reason why the API is currently
> overcomplicated is: if totally arbitrary parts of the kernel can find
> themselves in the restricted address space, we have more work to do.
> (It's totally possible, but we don't wanna block initial submission on
> that work). The simplification is about demarcating what code is and
> isn't affected by ASI, so having this "region" kinda helps with that.
> Although, because NMIs can also be affected it's a bit of a fuzzy
> demarcation...

Not just NMIs, but other IRQs can also be in the restricted address space even 
in this initial version. But that is of course still significantly less in scope 
than the general case, so the demarcation of process-context code via 
asi_start()/asi_end() does indeed seem useful.

Thanks,
Junaid


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ