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
