Re: [Xen-devel] [PATCH] x86/sm{e, a}p: do not enable SMEP/SMAP in PV shim by default on AMD

From: andrew.cooper3@citrix.com
To: David Woodhouse <dwmw2@infradead.org>
Cc: George.Dunlap@eu.citrix.com, jbeulich@suse.com, julien@xen.org,
	roger.pau@citrix.com, sstabellini@kernel.org, wl@xen.org,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH] x86/sm{e, a}p: do not enable SMEP/SMAP in PV shim by default on AMD
Date: Thu, 12 Oct 2023 10:13:15 +0800	[thread overview]
Message-ID: <b978a111-4cfb-45f3-850a-a5051298abef@citrix.com> (raw)
In-Reply-To: <b4e8835566bc6e9becc4f8adea7bb3c6cc558db2.camel@infradead.org>

On 11/10/2023 7:34 pm, David Woodhouse wrote:
>> On 16/01/2020 16:00, Igor Druzhinin wrote:
>>> Due to AMD and Hygon being unable to selectively trap CR4 bit modifications
>>> running 32-bit PV guest inside PV shim comes with significant performance
>>> hit. Moreover, for SMEP in particular every time CR4.SMEP changes on context
>>> switch to/from 32-bit PV guest, it gets trapped by L0 Xen which then
>>> tries to perform global TLB invalidation for PV shim domain. This usually
>>> results in eventual hang of a PV shim with at least several vCPUs.
>>>
>>> Since the overall security risk is generally lower for shim Xen as it being
>>> there more of a defense-in-depth mechanism, choose to disable SMEP/SMAP in
>>> it by default on AMD and Hygon unless a user chose otherwise.
>>>
>>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>>> ---
>>> I'm a little bit on the fence with this one. We're having the same issue with
>>> general nested virt but I'm not inclined to trade security for a user in
>>> general case. Disabling it by default for PV shim only seems rather inocuous
>>> due to the use case restricion. I'd like to hear more opinions.
>> So everyone on the list is up to date with the discussion we had IRL.
>>
>> SMEP/SMAP are defence in depth measures.  We support running on hardware
>> without these features, and the overall result is the same, security wise.
>>
>> In the PV Shim case, there is only a single guest and nothing
>> interesting in Xen, data wise.  We specifically do not have the risk of
>> sideways data leakage from other guests to be worried about.
>>
>> We do however care for performance, and not taking a VMExit on every
>> SYSCALL/Interrupt/Exception makes a massive difference overall.
> FWIW when running the shim under KVM even on Intel, constantly frobbing
> the CR4.SMEP bit still performs awfully.
>
> (Yes, we should make KVM pass that bit through to its guest on Intel
> hardware, just as Xen does when it's the HVM host. cf.
> https://lore.kernel.org/kvm/7fba6d8fc3de0bcb86bf629a4f5b0217552fe999.camel@infradead.org/T/#m39a117e90f29fc862b78ec1441b761459e7be86a
> )
>
> But why does the shim even need to turn it off when switching to the
> guest context? Its guest isn't running in supervisor mode so surely it
> doesn't *matter* whether SMEP is enabled or not? Why not just leave it
> on at all times?

32bit PV kernels run in Ring1.  Which is supervisor and not user.

Some older PV kernels do execute on user pages, and don't like getting
SMEP faults when they didn't turn it on to begin with.

SMAP is far more horrible.  STAC/CLAC are CPL0 instructions despite POPF
(changing AC) having the same architectural effect, but again, the guest
kernel doesn't tolerate SMAP faults when it didn't know it was enabled.

~Andrew