Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 24 Mar 2023 12:39:15 -0700
In-Reply-To: <CAL715W+a=og5f-v1o+56wTDmLJr1HitgFzmvL276BuomZvS7YA@mail.gmail.com>
Mime-Version: 1.0
References: <20230113122910.672417-1-jiangshanlai@gmail.com>
 <167934153606.1941128.1026865175616779306.b4-ty@google.com> <CAL715W+a=og5f-v1o+56wTDmLJr1HitgFzmvL276BuomZvS7YA@mail.gmail.com>
Message-ID: <ZB38Y2HpvD+aD9fm@google.com>
Subject: Re: [PATCH] kvm: x86/mmu: Simplify pte_list_{add|remove}
From:   Sean Christopherson <seanjc@google.com>
To:     Mingwei Zhang <mizhang@google.com>
Cc:     linux-kernel@vger.kernel.org,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Lai Jiangshan <jiangshan.ljs@antgroup.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Mar 24, 2023, Mingwei Zhang wrote:
> On Thu, Mar 23, 2023 at 3:51=E2=80=AFPM Sean Christopherson <seanjc@googl=
e.com> wrote:
> >
> > On Fri, 13 Jan 2023 20:29:10 +0800, Lai Jiangshan wrote:
> > > Simplify pte_list_{add|remove} by ensuring all the non-head pte_list_=
desc
> > > to be full and addition/removal actions being performed on the head.
> > >
> > > To make pte_list_add() return a count as before, @tail_count is also
> > > added to the struct pte_list_desc.
> > >
> > > No visible performace is changed in tests.  But pte_list_add() is no =
longer
> > > shown in the perf result for the COWed pages even the guest forks mil=
lions
> > > of tasks.
> > >
> > > [...]
> >
> > Applied to kvm-x86 mmu, thanks!  I added quite a few comments and a BUG=
_ON() to
> > sanity check that the head is never empty when trying to remove an entr=
y, but I
> > didn't make anything changes to the code itself.
> >
> > [1/1] kvm: x86/mmu: Simplify pte_list_{add|remove}
> >       https://github.com/kvm-x86/linux/commit/141705b78381
> >
>=20
> I am not sure if it is possible, but now spte_count is u32 so does
> tail_count. I wonder if an attacker could use the potential integer
> overflow to trigger this? E.g,: creating a huge number of little L1
> EPTs with the many nGPA-> one GPA? hmm, I think it could overflow
> tail_count? Please double check.

Heh, I had the same reaction and even started reworking the patch, but conv=
inced
myself that there are no new issues as __rmap_add() artificially limits the=
 number
of rmap entries to 1000.  But looking again, mmu_page_add_parent_pte() does=
n't
have the same safeguard, so in theory, a VM could overflow tail_count.

I'll stare at this some more next week, I'd really prefer to avoid taking o=
n any
amount of complexity in KVM to handle something that should never occur in
practice.  But being buried deep in __link_shadow_page() makes the error ha=
ndling
annoying.

> spte_count is u32, but assigned to an (signed) int j and BUG_ON(j <
> 0)? Please don't add more BUG_ON in KVM mmu... and please change
> either 'spte_count' to 'int' or 'j' to u32.

Why?  signed vs. unsigned doesn't change anything.  spte_count is can't be =
greater
than PTE_LIST_EXT, so overflow is not a concern.  The BUG_ON() is purely to=
 ensure
there's no underflow, i.e. that spte_count > 0, so that the array can be sa=
fely
accessed.

> In general, please, no BUG_ON(), at least no more BUG_ON() on our nested =
MMU...

In this case, the alternative is worse, as writing "head_desc->sptes[j]" wi=
th
"j < 0", i.e. a very large value if j is stored as an unsigned, would write
arbitrary kernel memory.

I completely agree that the BUG_ON() is gross, but that's an existing issue=
 that
needs to be remedied.  And to do that, I really want to resurrect and utili=
ze
the KVM_BUG_ON_DATA_CORRUPTION() idea[*].

[*] https://lore.kernel.org/all/Y5dax8XJV0F5adUw@google.com