NACK: [SRU][Bionic][PULL] KVM_VCPU_FLUSH_TLB for CVE-2019-3016

[Thadeu Lima de Souza Cascardo]

On Wed, Jul 15, 2020 at 12:22:50PM -0500, Alex Thorlton wrote:
> On Thu, Jun 25, 2020 at 06:46:50PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > On Thu, Jun 25, 2020 at 01:58:23PM -0700, Kamal Mostafa wrote:
> > > BugLink: https://urldefense.com/v3/__https://bugs.launchpad.net/bugs/1885184__;!!GqivPVa7Brio!OwuDG0uHgt9Eo0S10XTGHdes7ZcJ0ol1YltNO9QYYgsBeO8R-7K75l2tWH3WB7j4_AI$ 
> > > 
> > > As reported by Alex Thorlton <alex.thorlton at oracle.com>:
> > > 
> > > This mainline commit (part of the fix for CVE-2019-3016) was accidentally
> > > omitted from Bionic when the rest of that fix was applied in 4.15.0-106.107:
> > > 
> > > b043138246a4 x86/KVM: Make sure KVM_VCPU_FLUSH_TLB flag is not missed
> > > 
> > > This pull req supplies a backport of that commit (from its 4.19-stable
> > > backport) and its prequisites.
> > > 
> > >  -Kamal
> > 
> > I am going to NACK this for now. The thing is that the fix is not necessary
> > because the vulnerability is introduced by
> > f38a7b75267f1fb240a8178cbcb16d66dd37aac8 ("KVM: X86: support paravirtualized
> > help for TLB shootdowns") in the first place. Ironically, it's included in your
> > list of pre-requisites.
> > 
> > Of course, with the complete list of fixes, we would not be vulnerable. The
> > vulnerability also requires that guests use the feature, and, by default, on
> > bionic, qemu, as we ship it, does not enable that feature.
> > 
> > What we could discuss here is if it's worth it to bring the feature to bionic,
> > and whether it's right to backport any of the changes if not bringing
> > f38a7b75267f1fb2.
> 
> In doing further review of this CVE, and in light of Cascardo's comments
> here, I'm starting to wonder if Bionic was ever vulnerable to this CVE?
> Even if the feature was enabled in Bionic's qemu, as Cascardo mentioned,
> none of the following commits exist on Bionic:
> 
> > > Wanpeng Li (3):
> > >       KVM: X86: support paravirtualized help for TLB shootdowns
> > >       KVM: X86: Add KVM_VCPU_PREEMPTED
> > >       KVM: X86: use paravirtualized TLB Shootdown
> 
> The original fix that got pulled into Bionic for this CVE seems to have
> appeared on 4.15.0-96.97 in the form of:
> 
> x86/kvm: Be careful not to clear KVM_VCPU_FLUSH_TLB bit
> 
> But the commit that introduced the KVM_VCPU_FLUSH_TLB bit was:
> 
> KVM: X86: use paravirtualized TLB Shootdown
> 
> So, AFAICT, the Bionic kernel never had the necessary commits to make it
> vulnerable to the CVE, even before the new commits related to that CVE
> were introduced in 4.15.0-106.107.
> 
> Can you comment on this?  I'm not sure that anything really needs to be
> done here, I just found this a bit odd and wanted to get further
> clarification on the issue to make sure I understand what I'm seeing.
> 
> Thanks for the help so far!
> 
> - Alex

Hi, Alex.

Your analysis is correct. The reason those commits are being pulled is because
Kamal follows up what goes into some of the stable trees and commits that have
Fixes for commits that are included in our kernels, and some of those commits
end up being backported.

As those particular commits do not have Fixes tags, but have a reference to a
CVE, maybe that was what triggered their inclusion, but that wouldn't explain
why we missed commit "b043138246a4 x86/KVM: Make sure KVM_VCPU_FLUSH_TLB flag
is not missed". Possibly because its backport was not as straightforward.

Anyway, as you said, nothing that needs to be done here. Thanks a lot for your
report, though.

Cascardo.