On Wed, Jan 19, 2022 at 12:17:22PM +0100, Borislav Petkov wrote: > On Tue, Jan 18, 2022 at 07:18:06PM -0600, Michael Roth wrote: > > If 'fake_count'/'reported_count' is greater than the actual number of > > entries in the table, 'actual_count', then all table entries up to > > 'fake_count' will also need to pass validation. Generally the table > > will be zero'd out initially, so those additional/bogus entries will > > be interpreted as a CPUID leaves where all fields are 0. Unfortunately, > > that's still considered a valid leaf, even if it's a duplicate of the > > *actual* 0x0 leaf present earlier in the table. The current code will > > handle this fine, since it scans the table in order, and uses the > > valid 0x0 leaf earlier in the table. > > I guess it would be prudent to have some warnings when enumerating those > leafs and when the count index "goes off into the weeds", so to speak, > and starts reading 0-CPUID entries. I.e., "dear guest owner, your HV is > giving you a big lie: a weird/bogus CPUID leaf count..." > > :-) Ok, there's some sanity checks that happen a little later in boot via snp_cpuid_check_status(), after printk is enabled, that reports some basic details to dmesg like the number of entries in the table. I can add some additional sanity checks to flag the above case (really, all-zero entries never make sense, since CPUID 0x0 is supposed to report the max standard-range CPUID leaf, and leaf 0x1 at least should always be present). I'll print a warning for such cases, add maybe dump the cpuid the table in that case so it can be examined more easily by owner. > > And lemme make sure I understand it: the ->count itself is not > measured/encrypted because you want to be flexible here and supply > different blobs with different CPUID leafs? Yes, but to be clear it's the entire CPUID page, including the count, that's not measured (though it is encrypted after passing PSP validation). Probably the biggest reason is the logistics of having untrusted cloud vendors provide a copy of the CPUID values they plan to pass to the guest, since a new measurement would need to be calculated for every new configuration (using different guest cpuflags, SMP count, etc.), since those table values will need to be made easily-accessible to guest owner for all these measurement calculations, and they can't be trusted so each table would need to be checked either manually or by some tooling that could be difficult to implement unless it was something simple like "give me the expected CPUID values and I'll check if the provided CPUID table agrees with that". At that point it's much easier for the guest owner to just check the CPUID values directly against known good values for a particular configuration as part of their attestation process and leave the untrusted cloud vendor out of it completely. So not measuring the CPUID page as part of SNP attestation allows for that flexibility. > > > This is isn't really a special case though, it falls under the general > > category of a hypervisor inserting garbage entries that happen to pass > > validation, but don't reflect values that a guest would normally see. > > This will be detectable as part of guest owner attestation, since the > > guest code is careful to guarantee that the values seen after boot, > > once the attestation stage is reached, will be identical to the values > > seen during boot, so if this sort of manipulation of CPUID values > > occurred, the guest owner will notice this during attestation, and can > > abort the boot at that point. The Documentation patch addresses this > > in more detail. > > Yap, it is important this is properly explained there so that people can > pay attention to during attestation. > > > If 'fake_count' is less than 'actual_count', then the PSP skips > > validation for anything >= 'fake_count', and leaves them in the table. > > That should also be fine though, since guest code should never exceed > > 'fake_count'/'reported_count', as that's a blatant violation of the > > spec, and it doesn't make any sense for a guest to do this. This will > > effectively 'hide' entries, but those resulting missing CPUID leaves > > will be noticeable to the guest owner once attestation phase is > > reached. > > Noticeable because the guest owner did supply a CPUID table with X > entries but the HV is reporting Y? Or even more simply by the guest owner simply running 'cpuid -r -1' on the guest after boot, and making sure all the expected entries are present. If the HV manipulated the count to be lower, there would be missing entries, if they manipulated it to be higher, then there would either be extra duplicate entries at the end of the table (which the #VC handler would ignore due to it using the first matching entry in the table when doing lookups), or additional non-duplicate garbage entries, which will show up in 'cpuid -r -1' as unexpected entries. Really 'cpuid -r -1' is the guest owner/userspace view of things, so some of these nuances about the table contents might be noteworthy, but wouldn't actually affect guest behavior, which would be the main thing attestation process should be concerned with. > > If so, you can make this part of the attestation process: guest owners > should always check the CPUID entries count to be of a certain value. > > > This does all highlight the need for some very thorough guidelines > > on how a guest owner should implement their attestation checks for > > cpuid, however. I think a section in the reference implementation > > notes/document that covers this would be a good starting point. I'll > > also check with the PSP team on tightening up some of these CPUID > > page checks to rule out some of these possibilities in the future. > > Now you're starting to grow the right amount of paranoia - I'm glad I > was able to sensitize you properly! > > :-))) Hehe =*D Thanks! -Mike
