On Thu, Jan 09, 2025, Gavin Guo wrote:
>
> On 1/7/25 04:27, Sean Christopherson wrote:
> > On Sat, Dec 28, 2024, Sasha Levin wrote:
> > > On Thu, Dec 26, 2024 at 11:38:47AM +0800, Gavin Guo wrote:
> > > > From: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > >
> > > > [ Upstream commit 4b7c3f6d04bd53f2e5b228b6821fb8f5d1ba3071 ]
> > > >
> > > > Ignore the userspace provided x2APIC ID when fixing up APIC state for
> > > > KVM_SET_LAPIC, i.e. make the x2APIC fully readonly in KVM. Commit
> > > > a92e2543d6a8 ("KVM: x86: use hardware-compatible format for APIC ID
> > > > register"), which added the fixup, didn't intend to allow userspace to
> > > > modify the x2APIC ID. In fact, that commit is when KVM first started
> > > > treating the x2APIC ID as readonly, apparently to fix some race:
...
> > > > Reported-by: Michal Luczaj <mhal@xxxxxxx>
> > > > Closes: https://lore.kernel.org/all/814baa0c-1eaa-4503-129f-059917365e80@xxxxxxx
> > > > Reported-by: Haoyu Wu <haoyuwu254@xxxxxxxxx>
> > > > Closes: https://lore.kernel.org/all/20240126161633.62529-1-haoyuwu254@xxxxxxxxx
> > > > Reported-by: syzbot+545f1326f405db4e1c3e@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Closes: https://lore.kernel.org/all/000000000000c2a6b9061cbca3c3@xxxxxxxxxx
> > > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > > Message-ID: <20240802202941.344889-2-seanjc@xxxxxxxxxx>
> > > > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > > Signed-off-by: Gavin Guo <gavinguo@xxxxxxxxxx>
> > > As this one isn't tagged for stable, the KVM maintainers should ack the
> > > backport before we take it.
> > What's the motivation for applying this to 6.6? AFAIK, there's no real world use
> > case that benefits from the patch, the fix is purely to plug a hole where fuzzers,
> > e.g. syzkaller, can trip a WARN.
> >
> > That said, this is essentially a prerequisite for "KVM: x86: Re-split x2APIC ICR
> > into ICR+ICR2 for AMD (x2AVIC)"[*], and it's relatively low risk, so I'm not
> > opposed to landing it in 6.6.
> >
> > [*] https://lore.kernel.org/all/2024100123-unreached-enrage-2cb1@gregkh
>
> Thank you for reviewing the backporting. This backporting aims to address the
> syzkaller warning message and ensure that the stable kernel is consistent
> with the upstream version.
IMO, that's not sufficient justification for backporting to stable kernels. This
does not fix a problem that any real VMM will ever encounter, there's no security
implications, and outside of kernels that are running with panic_on_warn=1, which
seems insane outside of explicit test environments, there is zero risk to the host.
This is a bit of a bad example because I do expect this commit to land in 6.6 at
some point because it's a preqreq. But I don't want to set the precedent that
every commit that addresses a fuzzer-induced WARN is stable material.
Sanity check WARNs in KVM are tricky because userspace can shove arbitrary guest
state into KVM, i.e. avoiding what are effectively false positives requires quite
a bit of thought, and practically speaking such false positives will happen from
time to time. While I definitely want to keep KVM warning-free, I also don't want
to discourage sanity checks in KVM, i.e. I view intermittent false positives in
KVM as a cost of doing business.
If we start treating what are effectively false positives as stable material, then
it changes the math, i.e. each false positive becomes more costly because there
are backporting implications, and we'll naturally be more conservative in adding
sanity checks, which I don't want to happen.
