Re: [PATCH v9 38/43] x86/sev: Use firmware-validated CPUID for SEV-SNP guests

[Michael Roth <michael.roth@amd.com>]

On Sun, Feb 06, 2022 at 04:46:08PM +0100, Borislav Petkov wrote:
> On Sat, Feb 05, 2022 at 11:19:01AM -0600, Michael Roth wrote:
> > I mentioned the concern you raised about out-of-spec hypervisors
> > using non-zero for Reserved fields, and potentially breaking future
> > guests that attempt to make use of them if they ever get re-purposed
> > for some other functionality, but their take on that is that such a
> > hypervisor is clearly out-of-spec, and would not be supported.
> 
> Yah, like stating that something should not be done in the spec is
> going to stop people from doing it anyway. You folks need to understand
> one thing: the smaller the attack surface, the better. And you need to
> *enforce* that - not state it in a spec. No one cares about the spec
> when it comes to poking holes in the architecture. And people do poke
> and will poke holes *especially* in this architecture, as its main goal
> is security.

Agreed, but SNP never relies on the hypervisor to be the single authority
on what security features are available, if these fields were ever
re-purposed for anything in the future that would almost assuredly be
accompanied by a new SEV status MSR bit (that can't be intercepted),
a new policy bit (which affects measurement), a new "type2" CPUID page
that would effect measurement (contents don't affect measurement in
current firmware, but the metadata, like what type of page it is, is),
etc.

At that point any guest code changes to make use of those new fields
could then fail any out-of-spec hypervisors trying to use those fields
without the requisite firmware/hardware support. So please don't take
my summary as an indication that the security relies on hypervisors
abiding by the spec, this is more a statement that an out-of-spec
hypervisor should not expect that their guests will continue working
in future firmware versions, and what's being determined here is
whether to break those out-of-spec hypervisor now, or later when/if
we actually make use of the fields in the guest code, and whether we
need to make clarifications in the spec to help implementations avoid
such breakage.

> 
> > Another possibility is enforcing 0 in the firmware itself.
> 
> Yes, this is the thing to do. If they're going to be reused in the
> future, then guests can be changed to handle that. Like we do all the
> time anyway.

Ok, I'll follow up with the firmware team on this. But just to be clear,
what they're suggesting is that the firmware could enforce the MBZ checks
on the CPUID page, so out-of-spec hypervisors will fail immediately,
rather than in some future version of the spec/cpuid page, and guests
can continue ignoring them in the meantime.

I'll also note the type you spotted with the Table 13 reference and
see if there's anything else that can be cleared up there.

> 
> > So given their guidance on the Reserved fields, and your guidance
> > on not doing the other sanity-checks, my current plan to to drop
> > snp_check_cpuid_table() completely for v10, but if you have other
> > thoughts on that just let me know.
> 
> Yes, and pls fix the firmware to zero them out, because from reading
> your very detailed explanation - btw, thanks for taking the time to
> explain properly what's not in the ABI doc:
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20220205154243.s33gwghqwbb4efyl%40amd.com&data=04%7C01%7Cmichael.roth%40amd.com%7C377208b805be40f55c0f08d9e987d19e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637797591858315372%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=L9McNfcn514uHmHLCtFPI2TqLKoCK0%2FKDMPk32lO8r8%3D&reserved=0
> 
> it sounds like those two input fields are not really needed. So the
> earlier you fix them in the firmware and deprecate them, the better.

XCR0_IN/XSS_IN aren't needed by a guest if it follows the recommended
implementation and computes them on the fly, but they do still serve
a purpose in the context of firmware validation of 0xD,0x0/0xD,0x1
leaves, since those are validated relative to a particular
XCR0_IN/XSS_IN.

Whether a guest chooses to use the resulting firmware-validated version
of EBX for those, or compute it on it's own, is considered outside the
scope of the SNP firmware ABI, so I think the plan is to leave those
fields in the spec at least for current SNP version, and rely on the
implementation recommendations to document anything outside of that.

But since the recommendations need to be compatibile with the SNP
firmware ABI, I've updated the recommendations to have the guest to go
ahead and check for XCR0_IN={1,3}/XSS_IN=0 when searching for base
0xD,0x0/0xD,0x1 entries, so that there's no question of whether a guest
is supposed to expect XCR0_IN/XSS_IN to be 0.

Getting that document ready to send if a few.

Hope that helps clear things up, but please let me know if there's
anything else that needs clarification.

Thanks!

-Mike

> 
> Thx!
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7C377208b805be40f55c0f08d9e987d19e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637797591858315372%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=0uNI0Ojku3ZIgiQRbNf0UxNXruycXsOYIUNIY9I2IiY%3D&reserved=0