Re: [PATCH 1/2] KVM: arm64: vgic: check redist region is not above the VM IPA size

Hi Alexandru, Eric,

On Mon, Sep 13, 2021 at 11:15:33AM +0100, Alexandru Elisei wrote:

Hi Eric, Ricardo,

On 9/10/21 20:32, Ricardo Koller wrote:

quoted

Hi Alexandru and Eric,

On Fri, Sep 10, 2021 at 10:42:23AM +0200, Eric Auger wrote:

quoted

Hi Alexandru,

On 9/10/21 10:28 AM, Alexandru Elisei wrote:

quoted

Hi Ricardo,

On 9/9/21 5:47 PM, Ricardo Koller wrote:

quoted

On Thu, Sep 09, 2021 at 11:20:15AM +0100, Alexandru Elisei wrote:

quoted

Hi Ricardo,

On 9/8/21 10:03 PM, Ricardo Koller wrote:

quoted

Extend vgic_v3_check_base() to verify that the redistributor regions
don't go above the VM-specified IPA size (phys_size). This can happen
when using the legacy KVM_VGIC_V3_ADDR_TYPE_REDIST attribute with:

  base + size > phys_size AND base < phys_size

vgic_v3_check_base() is used to check the redist regions bases when
setting them (with the vcpus added so far) and when attempting the first
vcpu-run.

Signed-off-by: Ricardo Koller <redacted>
---
 arch/arm64/kvm/vgic/vgic-v3.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c
index 66004f61cd83..5afd9f6f68f6 100644
--- a/arch/arm64/kvm/vgic/vgic-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-v3.c

@@ -512,6 +512,10 @@ bool vgic_v3_check_base(struct kvm *kvm)
 		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) <
 			rdreg->base)
 			return false;
+
+		if (rdreg->base + vgic_v3_rd_region_size(kvm, rdreg) >
+			kvm_phys_size(kvm))
+			return false;

Looks to me like this same check (and the overflow one before it) is done when
adding a new Redistributor region in kvm_vgic_addr() -> vgic_v3_set_redist_base()
-> vgic_v3_alloc_redist_region() -> vgic_check_ioaddr(). As far as I can tell,
kvm_vgic_addr() handles both ways of setting the Redistributor address.

Without this patch, did you manage to set a base address such that base + size >
kvm_phys_size()?

Yes, with the KVM_VGIC_V3_ADDR_TYPE_REDIST legacy API. The easiest way
to get to this situation is with the selftest in patch 2.  I then tried
an extra experiment: map the first redistributor, run the first vcpu,
and access the redist from inside the guest. KVM didn't complain in any
of these steps.

Yes, Eric pointed out that I was mistaken and there is no check being done for
base + size > kvm_phys_size().

What I was trying to say is that this check is better done when the user creates a
Redistributor region, not when a VCPU is first run. We have everything we need to
make the check when a region is created, why wait until the VCPU is run?

For example, vgic_v3_insert_redist_region() is called each time the adds a new
Redistributor region (via either of the two APIs), and already has a check for the
upper limit overflowing (identical to the check in vgic_v3_check_base()). I would
add the check against the maximum IPA size there.

you seem to refer to an old kernel as vgic_v3_insert_redist_region was
renamed into� vgic_v3_alloc_redist_region in
e5a35635464b kvm: arm64: vgic-v3: Introduce vgic_v3_free_redist_region()

I think in case you use the old rdist API you do not know yet the size
of the redist region at this point (count=0), hence Ricardo's choice to
do the check latter.

Just wanted to add one more detail. vgic_v3_check_base() is also called
when creating the redistributor region (via vgic_v3_set_redist_base ->
vgic_register_redist_iodev). This patch reuses that check for the old
redist API to also check for "base + size > kvm_phys_size()" with a size
calculated using the vcpus added so far.

@Eric: Indeed I was looking at an older kernel by mistake, thank you for pointing
that out!

Thank you both for the explanations, the piece I was missing was the fact that
KVM_VGIC_V3_ADDR_TYPE_REDIST specifies only the base address and the limit for the
region is the number of VCPUs * (KVM_VGIC_V3_REDIST_SIZE = 128K), which makes it
necessary to have the check when each VCPU is first run (as far as I can tell,
VCPUs can be created at any time).

quoted

quoted

quoted

Also, because vgic_v3_insert_redist_region() already checks for overflow, I
believe the overflow check in vgic_v3_check_base() is redundant.

It's redundant for the new redist API, but still needed for the old
redist API.

Indeed.

quoted

quoted

quoted

As far as I can tell, vgic_v3_check_base() is there to make sure that the
Distributor doesn't overlap with any of the Redistributors, and because the
Redistributors and the Distributor can be created in any order, we defer the check
until the first VCPU is run. I might be wrong about this, someone please correct
me if I'm wrong.

Also, did you verify that KVM is also doing this check for GICv2? KVM does
something similar and calls vgic_v2_check_base() when mapping the GIC resources,
and I don't see a check for the maximum IPA size in that function either.

I think vgic_check_ioaddr() called in kvm_vgic_addr() does the job (it
checks the base @)

It seems that GICv2 suffers from the same problem. The cpu interface
base is checked but the end can extend above IPA size. Note that the cpu
interface is 8KBs and vgic_check_ioaddr() is only checking that its base

... except that the doc for KVM_VGIC_V2_ADDR_TYPE_CPU says that the CPU interface
region is 4K, while the check in vgic_v2_check_base() is done against
KVM_VGIC_V2_CPU_SIZE, which is 8K.

The "GIC virtual CPU interface" alone is slightly more than 4K: GICV_DIR
is at 0x1000. The documentation might need to be updated.

I suppose that the CPU interface region is 8K
because ARM IHI 0048B.b strongly recommends that the virtual CPU interface control
registers are in a separate 4KB region, and KVM wants to emulate a GICv2 as close
to the real thing as possible?

Are the "virtual CPU interface control" registers the ones starting with
GICH_? If yes, then I'm a bit confused, as those are not exposed to the
guest (to my knowledge).

quoted

is 4KB aligned and below IPA size. The distributor region is 4KB so
vgic_check_ioaddr() is enough in that case.

What about the following?

I can work on the next version of this patch (v2 has the GICv2 issue)
which adds vgic_check_range(), which is like vgic_check_ioaddr() but
with a size arg.  kvm_vgic_addr() can then call vgic_check_range() and
do all the checks for GICv2 and GICv3. Note that for GICv2, there's no
need to wait until first vcpu run to do the check. Also note that I will
have to keep the change in vgic_v3_check_base() to check for the old v3
redist API at first vcpu run.

Sounds good.

Thanks,

Alex

quoted

Thanks,
Ricardo

quoted

Thanks

Eric

Will do, thank you both.

Ricardo

quoted

quoted

quoted

Thanks,

Alex

quoted

Thanks,
Ricardo

quoted

Thanks,

Alex

quoted

 	}
 
 	if (IS_VGIC_ADDR_UNDEF(d->vgic_dist_base))