Hi Ard, Thanks for your explanation! On Thu, Jul 13, 2023 at 6:18 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > On Fri, 7 Jul 2023 at 19:12, Borislav Petkov <bp@xxxxxxxxx> wrote: > > > > On Fri, Jul 07, 2023 at 10:25:15AM -0500, Michael Roth wrote: > > > ... > > > It would be unfortunate if we finally abandoned this path because of the > > > issue being hit here though. I think the patch posted here is the proper > > > resolution to the issue being hit, and I'm hoping at this point we've > > > identified all the similar cases where EFI/setup_data-related structures > > > were missing explicit mappings. But if we still think it's too much of a > > > liability to access the EFI config table outside of SEV-enabled guests, > > > then I can work on re-implementing things based on the above logic. > > > > Replying here to Tom's note too... > > > > So, I like the idea of rechecking CPUID. Yes, let's do the sev_status > > check. As a result, we either fail the guest - no problem - or we boot > > and we recheck. Thus, we don't run AMD code on !AMD machines, if the HV > > is not a lying bastard. > > > > Now, if we've gotten a valid setup_data SETUP_EFI entry with a valid > > pointer to an EFI config table, then that should happen in the generic > > path - initialize_identity_maps(), for example - like you've done in > > b57feed2cc26 - not in the kexec code because kexec *happens* to need it. > > > > We want to access the EFI config table? Sure, by all means, but make > > that generic for all code. > > > > OK, so in summary, what seems to be happening here is that the SEV > init code in the decompressor looks for the cc blob table before the > on-demand mapping code is up, which normally ensures that any RAM > address is accessible even if it hasn't been mapped explicitly. > Yes it is exactly the case. > This is why the fix happens to work: the code only maps the array of > (guid, phys_addr) tuples that describes the list of configuration > tables that have been provided by the firmware. The actual > configuration tables themselves could be anywhere in physical memory, > and without prior knowledge of a particular GUID value, there is no > way to know the size of the table, and so they cannot be mapped Should we loop map each element of the config table one at a time? We read a GUID value, then we map it with phys_addr, phys_addr + sizeof(struct), then we read-map the next one, in this way we may not need to know the size of the table? > upfront like this. However, the cc blob table does not exist on this > machine, and so whether the EFI config tables themselves are mapped or > not is irrelevant. Currently we don't need all the data of the config table, only part of it. So only (guid, phys_addr) tuple arrays are mapped into. Won't it be better if we map config table on demand if we need further data later? > > But it does mean the fix is incomplete, and certainly does not belong > in generic kexec code. If anything, we should be fixing the > decompressor code to defer the cc blob table check until after the > demand mapping code is up. Yes, if we can defer the cc blob access, the issue can be resolved. I don't know if it is doable from AMD's view... > > If this is problematic, we might instead disable SEV for kexec, and > rely on the fact that SEV firmware enters with a complete 1:1 map (as We still need sev support for kexec. If we disable sev during kexec, I suppose kdump may be broken on a sev guest, maybe? Thanks, Tao Liu > we seem to be doing currently). If kexec for SEV is needed at some > point, we can re-enable it by having it provide a mapping for the > config table array and the cc blob table explicitly. >
