On Thu, Oct 26, 2023 at 02:22:58PM -0700, Sean Christopherson wrote: > On Thu, Oct 26, 2023, Pawan Gupta wrote: > > On Thu, Oct 26, 2023 at 12:30:55PM -0700, Sean Christopherson wrote: > > > > if (static_branch_unlikely(&vmx_l1d_should_flush)) > > > > vmx_l1d_flush(vcpu); > > > > > > There's an existing bug here. vmx_1ld_flush() is not guaranteed to do a flush in > > > "conditional mode", and is not guaranteed to do a ucode-based flush > > > > AFAICT, it is based on the condition whether after a VMexit any > > sensitive data could have been touched or not. If L1TF mitigation > > doesn't consider certain data sensitive and skips L1D flush, executing > > VERW isn't giving any protection, since that data can anyways be leaked > > from L1D using L1TF. > > That assumes vcpu->arch.l1tf_flush_l1d is 100% precise and accurate, which is most > definitely not the case. You're also preventing the admin from choosing between > being super paranoind (always flush L1D) and mostly paranoid (conditionally flush > L1D, always flush CPU buffers). > AIUI, flushing the L1D is crazy expensive compared to flushing the CPU buffers, > so it's entirely plausible for someone to want to choose the mostly paranoid > option. Sure, if it helps an admin. I was asking about the problematic scenario out of curiosity. BTW, the changes you suggested are definitely worth doing. > Side topic, isn't the NMI path missing a call to kvm_set_cpu_l1tf_flush_l1d()? Yes, it is missing. Not sure if it was omitted intentionally. > > This is certainly better, but I don't know what scenario is this helping with. > > Heh, that's host I feel about moving VERW to just before VM-Enter. I have a hard > time believing there's meaningful sensitive that's accessed in __vmx_vcpu_run(). > The closest thing is probably CR2, but that's a very dubious vector since CR2 will > hold a guest value for most VM-Enters. Yes, kernel->user case has a better chance of leaking anything. > I'm not against moving VERW close to VM-Enter because it's relatively straightforward, > but if we're going to be super paranoid, why not go all the way and not have to > worry about what ifs? Right. The VMenter changes are mostly done to be consistent with what is being done for kernel->user.
