kvmarm.lists.cs.columbia.edu archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] KVM: arm64: vgic: Fix handling of userspace register accesses
@ 2020-11-13 14:27 Zenghui Yu
  2020-11-13 14:28 ` [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses Zenghui Yu
  2020-11-13 14:28 ` [PATCH 2/2] KVM: arm64: vgic: Forbid invalid userspace Distributor accesses Zenghui Yu
  0 siblings, 2 replies; 12+ messages in thread
From: Zenghui Yu @ 2020-11-13 14:27 UTC (permalink / raw)
  To: kvmarm, maz; +Cc: linux-kernel, linux-arm-kernel

We had recently seen a kernel panic when accidently programming QEMU in an
inappropriate way (in short, accessing RD registers before setting the RD
base address. See patch #1 for details). And it looks like we're missing
some basic checking when handling userspace register access.

I've only tested it with QEMU. It'd be appreciated if others can test it
with other user tools.

Zenghui Yu (2):
  KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  KVM: arm64: vgic: Forbid invalid userspace Distributor accesses

 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.19.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-13 14:27 [PATCH 0/2] KVM: arm64: vgic: Fix handling of userspace register accesses Zenghui Yu
@ 2020-11-13 14:28 ` Zenghui Yu
  2020-11-15 17:04   ` Marc Zyngier
  2020-11-13 14:28 ` [PATCH 2/2] KVM: arm64: vgic: Forbid invalid userspace Distributor accesses Zenghui Yu
  1 sibling, 1 reply; 12+ messages in thread
From: Zenghui Yu @ 2020-11-13 14:28 UTC (permalink / raw)
  To: kvmarm, maz; +Cc: linux-kernel, linux-arm-kernel

It's expected that users will access registers in the redistributor *if*
the RD has been initialized properly. Unfortunately userspace can be bogus
enough to access registers before setting the RD base address, and KVM
implicitly allows it (we handle the access anyway, regardless of whether
the base address is set).

Bad thing happens when we're handling the user read of GICR_TYPER. We end
up with an oops when deferencing the unset rdreg...

	gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
			(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;

Fix this issue by informing userspace what had gone wrong (-ENXIO).

Reported-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 52d6f24f65dc..30e370585a27 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -1040,11 +1040,15 @@ int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 int vgic_v3_redist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			   int offset, u32 *val)
 {
+	struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
 	struct vgic_io_device rd_dev = {
 		.regions = vgic_v3_rd_registers,
 		.nr_regions = ARRAY_SIZE(vgic_v3_rd_registers),
 	};
 
+	if (IS_VGIC_ADDR_UNDEF(vgic_cpu->rd_iodev.base_addr))
+		return -ENXIO;
+
 	return vgic_uaccess(vcpu, &rd_dev, is_write, offset, val);
 }
 
-- 
2.19.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/2] KVM: arm64: vgic: Forbid invalid userspace Distributor accesses
  2020-11-13 14:27 [PATCH 0/2] KVM: arm64: vgic: Fix handling of userspace register accesses Zenghui Yu
  2020-11-13 14:28 ` [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses Zenghui Yu
@ 2020-11-13 14:28 ` Zenghui Yu
  1 sibling, 0 replies; 12+ messages in thread
From: Zenghui Yu @ 2020-11-13 14:28 UTC (permalink / raw)
  To: kvmarm, maz; +Cc: linux-kernel, linux-arm-kernel

Accessing registers in the Distributor before setting its base address
should be treated as an invalid userspece behaviour. But KVM implicitly
allows it as we handle the access anyway, regardless of whether the base
address is set or not.

Fix this issue by informing userspace what had gone wrong (-ENXIO).

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arch/arm64/kvm/vgic/vgic-mmio-v3.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 30e370585a27..6a9e5eb311f0 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -1029,11 +1029,15 @@ void vgic_v3_dispatch_sgi(struct kvm_vcpu *vcpu, u64 reg, bool allow_group1)
 int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write,
 			 int offset, u32 *val)
 {
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
 	struct vgic_io_device dev = {
 		.regions = vgic_v3_dist_registers,
 		.nr_regions = ARRAY_SIZE(vgic_v3_dist_registers),
 	};
 
+	if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base))
+		return -ENXIO;
+
 	return vgic_uaccess(vcpu, &dev, is_write, offset, val);
 }
 
-- 
2.19.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-13 14:28 ` [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses Zenghui Yu
@ 2020-11-15 17:04   ` Marc Zyngier
  2020-11-16 13:09     ` Zenghui Yu
  0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2020-11-15 17:04 UTC (permalink / raw)
  To: Zenghui Yu; +Cc: linux-kernel, linux-arm-kernel, kvmarm

Hi Zenghui,

On 2020-11-13 14:28, Zenghui Yu wrote:
> It's expected that users will access registers in the redistributor 
> *if*
> the RD has been initialized properly. Unfortunately userspace can be 
> bogus
> enough to access registers before setting the RD base address, and KVM
> implicitly allows it (we handle the access anyway, regardless of 
> whether
> the base address is set).
> 
> Bad thing happens when we're handling the user read of GICR_TYPER. We 
> end
> up with an oops when deferencing the unset rdreg...
> 
> 	gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
> 			(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
> 
> Fix this issue by informing userspace what had gone wrong (-ENXIO).

I'm worried about the "implicit" aspect of the access that this patch
now forbids.

The problem is that the existing documentation doesn't cover this case,
and -ENXIO's "Getting or setting this register is not yet supported"
is way too vague. There is a precedent with the ITS, but that's 
undocumented
as well. Also, how about v2? If that's the wasy we are going to fix 
this,
we also nned to beef up the documentation.

Of course, the other horrible way to address the issue is to return a 
value
that doesn't have the Last bit set, since we can't synthetise it. It 
doesn't
change the userspace API, and I can even find some (admittedly  twisted)
logic to it (since there is no base address, there is no last RD...).

Thoughts?

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-15 17:04   ` Marc Zyngier
@ 2020-11-16 13:09     ` Zenghui Yu
  2020-11-16 14:10       ` Marc Zyngier
  0 siblings, 1 reply; 12+ messages in thread
From: Zenghui Yu @ 2020-11-16 13:09 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, linux-arm-kernel, kvmarm

Hi Marc,

On 2020/11/16 1:04, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On 2020-11-13 14:28, Zenghui Yu wrote:
>> It's expected that users will access registers in the redistributor *if*
>> the RD has been initialized properly. Unfortunately userspace can be 
>> bogus
>> enough to access registers before setting the RD base address, and KVM
>> implicitly allows it (we handle the access anyway, regardless of whether
>> the base address is set).
>>
>> Bad thing happens when we're handling the user read of GICR_TYPER. We end
>> up with an oops when deferencing the unset rdreg...
>>
>>     gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
>>             (rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
>>
>> Fix this issue by informing userspace what had gone wrong (-ENXIO).
> 
> I'm worried about the "implicit" aspect of the access that this patch
> now forbids.
> 
> The problem is that the existing documentation doesn't cover this case, > and -ENXIO's "Getting or setting this register is not yet supported"
> is way too vague.

Indeed. How about changing to

     -ENXIO  Getting or setting this register is not yet supported
             or VGIC not properly configured (e.g., [Re]Distributor base
             address is unknown)

> There is a precedent with the ITS, but that's 
> undocumented
> as well. Also, how about v2? If that's the wasy we are going to fix this,
> we also nned to beef up the documentation.

Sure, I plan to do so and hope it won't break the existing userspace.

> Of course, the other horrible way to address the issue is to return a value
> that doesn't have the Last bit set, since we can't synthetise it. It 
> doesn't
> change the userspace API, and I can even find some (admittedly  twisted)
> logic to it (since there is no base address, there is no last RD...).

I'm fine with it. But I'm afraid that there might be other issues due to
the "unexpected" accesses since I haven't tested with all registers from
userspace.

My take is that only if the "[Re]Distributor base address" is specified
in the system memory map, will the user-provided kvm_device_attr.offset
make sense. And we can then handle the access to the register which is
defined by "base address + offset".


Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-16 13:09     ` Zenghui Yu
@ 2020-11-16 14:10       ` Marc Zyngier
  2020-11-16 14:57         ` Zenghui Yu
  0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2020-11-16 14:10 UTC (permalink / raw)
  To: Zenghui Yu; +Cc: linux-kernel, linux-arm-kernel, kvmarm

On 2020-11-16 13:09, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2020/11/16 1:04, Marc Zyngier wrote:
>> Hi Zenghui,
>> 
>> On 2020-11-13 14:28, Zenghui Yu wrote:
>>> It's expected that users will access registers in the redistributor 
>>> *if*
>>> the RD has been initialized properly. Unfortunately userspace can be 
>>> bogus
>>> enough to access registers before setting the RD base address, and 
>>> KVM
>>> implicitly allows it (we handle the access anyway, regardless of 
>>> whether
>>> the base address is set).
>>> 
>>> Bad thing happens when we're handling the user read of GICR_TYPER. We 
>>> end
>>> up with an oops when deferencing the unset rdreg...
>>> 
>>>     gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
>>>             (rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
>>> 
>>> Fix this issue by informing userspace what had gone wrong (-ENXIO).
>> 
>> I'm worried about the "implicit" aspect of the access that this patch
>> now forbids.
>> 
>> The problem is that the existing documentation doesn't cover this 
>> case, > and -ENXIO's "Getting or setting this register is not yet 
>> supported"
>> is way too vague.
> 
> Indeed. How about changing to
> 
>     -ENXIO  Getting or setting this register is not yet supported
>             or VGIC not properly configured (e.g., [Re]Distributor base
>             address is unknown)

Looks OK to me.

> 
>> There is a precedent with the ITS, but that's undocumented
>> as well. Also, how about v2? If that's the wasy we are going to fix 
>> this,
>> we also nned to beef up the documentation.
> 
> Sure, I plan to do so and hope it won't break the existing userspace.

Well, at this stage we can only hope.

> 
>> Of course, the other horrible way to address the issue is to return a 
>> value
>> that doesn't have the Last bit set, since we can't synthetise it. It 
>> doesn't
>> change the userspace API, and I can even find some (admittedly  
>> twisted)
>> logic to it (since there is no base address, there is no last RD...).
> 
> I'm fine with it. But I'm afraid that there might be other issues due 
> to
> the "unexpected" accesses since I haven't tested with all registers 
> from
> userspace.

I have had a look at the weekend, and couldn't see any other other GICR
register that would suffer from rdreg being NULL. I haven't looked at
GICD, but I don't anticipate anything bad on that front.

> My take is that only if the "[Re]Distributor base address" is specified
> in the system memory map, will the user-provided kvm_device_attr.offset
> make sense. And we can then handle the access to the register which is
> defined by "base address + offset".

I'd tend to agree, but it is just that this is a large change at -rc4.
I'd rather have a quick fix for 5.10, and a more invasive change for 
5.11,
spanning all the possible vgic devices.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-16 14:10       ` Marc Zyngier
@ 2020-11-16 14:57         ` Zenghui Yu
  2020-11-17  8:49           ` Marc Zyngier
  0 siblings, 1 reply; 12+ messages in thread
From: Zenghui Yu @ 2020-11-16 14:57 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, linux-arm-kernel, kvmarm

Hi Marc,

On 2020/11/16 22:10, Marc Zyngier wrote:
>> My take is that only if the "[Re]Distributor base address" is specified
>> in the system memory map, will the user-provided kvm_device_attr.offset
>> make sense. And we can then handle the access to the register which is
>> defined by "base address + offset".
> 
> I'd tend to agree, but it is just that this is a large change at -rc4.
> I'd rather have a quick fix for 5.10, and a more invasive change for 5.11,
> spanning all the possible vgic devices.

So you prefer fixing it by "return a value that doesn't have the Last
bit set" for v5.10? I'm ok with it and can send v2 for it.

Btw, looking again at the way we handle the user-reading of GICR_TYPER

	vgic_mmio_read_v3r_typer(vcpu, addr, len)

it seems that @addr is actually the *offset* of GICR_TYPER (0x0008) and
@addr is unlikely to be equal to last_rdist_typer, which is the *GPA* of
the last RD. Looks like the user-reading of GICR_TYPER.Last is always
broken?


Thanks,
Zenghui
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-16 14:57         ` Zenghui Yu
@ 2020-11-17  8:49           ` Marc Zyngier
  2020-11-17  9:47             ` Auger Eric
                               ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Marc Zyngier @ 2020-11-17  8:49 UTC (permalink / raw)
  To: Zenghui Yu; +Cc: linux-kernel, linux-arm-kernel, kvmarm

Hi Zenghui,

On 2020-11-16 14:57, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2020/11/16 22:10, Marc Zyngier wrote:
>>> My take is that only if the "[Re]Distributor base address" is 
>>> specified
>>> in the system memory map, will the user-provided 
>>> kvm_device_attr.offset
>>> make sense. And we can then handle the access to the register which 
>>> is
>>> defined by "base address + offset".
>> 
>> I'd tend to agree, but it is just that this is a large change at -rc4.
>> I'd rather have a quick fix for 5.10, and a more invasive change for 
>> 5.11,
>> spanning all the possible vgic devices.
> 
> So you prefer fixing it by "return a value that doesn't have the Last
> bit set" for v5.10? I'm ok with it and can send v2 for it.

Cool. Thanks for that.

> Btw, looking again at the way we handle the user-reading of GICR_TYPER
> 
> 	vgic_mmio_read_v3r_typer(vcpu, addr, len)
> 
> it seems that @addr is actually the *offset* of GICR_TYPER (0x0008) and
> @addr is unlikely to be equal to last_rdist_typer, which is the *GPA* 
> of
> the last RD. Looks like the user-reading of GICR_TYPER.Last is always
> broken?

I think you are right. Somehow, we don't seem to track the index of
the RD in the region, so we can never compute the address of the RD
even if the base address is set.

Let's drop the reporting of Last for userspace for now, as it never
worked. If you post a patch addressing that quickly, I'll get it to
Paolo by the end of the week (there's another fix that needs merging).

Eric: do we have any test covering the userspace API?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-17  8:49           ` Marc Zyngier
@ 2020-11-17  9:47             ` Auger Eric
  2020-11-17  9:59             ` Auger Eric
  2020-11-17 13:09             ` Zenghui Yu
  2 siblings, 0 replies; 12+ messages in thread
From: Auger Eric @ 2020-11-17  9:47 UTC (permalink / raw)
  To: Marc Zyngier, Zenghui Yu; +Cc: linux-kernel, linux-arm-kernel, kvmarm

Hi Zenghui,

On 11/17/20 9:49 AM, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On 2020-11-16 14:57, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/11/16 22:10, Marc Zyngier wrote:
>>>> My take is that only if the "[Re]Distributor base address" is specified
>>>> in the system memory map, will the user-provided kvm_device_attr.offset
>>>> make sense. And we can then handle the access to the register which is
>>>> defined by "base address + offset".
>>>
>>> I'd tend to agree, but it is just that this is a large change at -rc4.
>>> I'd rather have a quick fix for 5.10, and a more invasive change for
>>> 5.11,
>>> spanning all the possible vgic devices.
>>
>> So you prefer fixing it by "return a value that doesn't have the Last
>> bit set" for v5.10? I'm ok with it and can send v2 for it.
> 
> Cool. Thanks for that.
> 
>> Btw, looking again at the way we handle the user-reading of GICR_TYPER
>>
>>     vgic_mmio_read_v3r_typer(vcpu, addr, len)
>>
>> it seems that @addr is actually the *offset* of GICR_TYPER (0x0008) and
>> @addr is unlikely to be equal to last_rdist_typer, which is the *GPA* of
>> the last RD. Looks like the user-reading of GICR_TYPER.Last is always
>> broken?
> 
> I think you are right. Somehow, we don't seem to track the index of
> the RD in the region, so we can never compute the address of the RD
> even if the base address is set.
> 
> Let's drop the reporting of Last for userspace for now, as it never
> worked. If you post a patch addressing that quickly, I'll get it to
> Paolo by the end of the week (there's another fix that needs merging).
> 
> Eric: do we have any test covering the userspace API?
No, there are no KVM selftests for that yet.

Thanks

Eric

> 
> Thanks,
> 
>         M.

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-17  8:49           ` Marc Zyngier
  2020-11-17  9:47             ` Auger Eric
@ 2020-11-17  9:59             ` Auger Eric
  2020-11-17 10:51               ` Marc Zyngier
  2020-11-17 13:09             ` Zenghui Yu
  2 siblings, 1 reply; 12+ messages in thread
From: Auger Eric @ 2020-11-17  9:59 UTC (permalink / raw)
  To: Marc Zyngier, Zenghui Yu; +Cc: linux-kernel, kvmarm, linux-arm-kernel

Hi Marc,

On 11/17/20 9:49 AM, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On 2020-11-16 14:57, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/11/16 22:10, Marc Zyngier wrote:
>>>> My take is that only if the "[Re]Distributor base address" is specified
>>>> in the system memory map, will the user-provided kvm_device_attr.offset
>>>> make sense. And we can then handle the access to the register which is
>>>> defined by "base address + offset".
>>>
>>> I'd tend to agree, but it is just that this is a large change at -rc4.
>>> I'd rather have a quick fix for 5.10, and a more invasive change for
>>> 5.11,
>>> spanning all the possible vgic devices.
>>
>> So you prefer fixing it by "return a value that doesn't have the Last
>> bit set" for v5.10? I'm ok with it and can send v2 for it.
> 
> Cool. Thanks for that.
> 
>> Btw, looking again at the way we handle the user-reading of GICR_TYPER
>>
>>     vgic_mmio_read_v3r_typer(vcpu, addr, len)
>>
>> it seems that @addr is actually the *offset* of GICR_TYPER (0x0008) and
>> @addr is unlikely to be equal to last_rdist_typer, which is the *GPA* of
>> the last RD. Looks like the user-reading of GICR_TYPER.Last is always
>> broken?
> 
> I think you are right. Somehow, we don't seem to track the index of
> the RD in the region, so we can never compute the address of the RD
> even if the base address is set.
> 
> Let's drop the reporting of Last for userspace for now, as it never
> worked. If you post a patch addressing that quickly, I'll get it to
> Paolo by the end of the week (there's another fix that needs merging).
> 
> Eric: do we have any test covering the userspace API?

So as this issue seems related to the changes made when implementing the
multiple RDIST regions, I volunteer to write those KVM selftests :-)

Thanks

Eric

> 
> Thanks,
> 
>         M.

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-17  9:59             ` Auger Eric
@ 2020-11-17 10:51               ` Marc Zyngier
  0 siblings, 0 replies; 12+ messages in thread
From: Marc Zyngier @ 2020-11-17 10:51 UTC (permalink / raw)
  To: Auger Eric; +Cc: linux-kernel, kvmarm, linux-arm-kernel

On 2020-11-17 09:59, Auger Eric wrote:
> Hi Marc,
> 
> On 11/17/20 9:49 AM, Marc Zyngier wrote:
>> Hi Zenghui,
>> 
>> On 2020-11-16 14:57, Zenghui Yu wrote:
>>> Hi Marc,
>>> 
>>> On 2020/11/16 22:10, Marc Zyngier wrote:
>>>>> My take is that only if the "[Re]Distributor base address" is 
>>>>> specified
>>>>> in the system memory map, will the user-provided 
>>>>> kvm_device_attr.offset
>>>>> make sense. And we can then handle the access to the register which 
>>>>> is
>>>>> defined by "base address + offset".
>>>> 
>>>> I'd tend to agree, but it is just that this is a large change at 
>>>> -rc4.
>>>> I'd rather have a quick fix for 5.10, and a more invasive change for
>>>> 5.11,
>>>> spanning all the possible vgic devices.
>>> 
>>> So you prefer fixing it by "return a value that doesn't have the Last
>>> bit set" for v5.10? I'm ok with it and can send v2 for it.
>> 
>> Cool. Thanks for that.
>> 
>>> Btw, looking again at the way we handle the user-reading of 
>>> GICR_TYPER
>>> 
>>>     vgic_mmio_read_v3r_typer(vcpu, addr, len)
>>> 
>>> it seems that @addr is actually the *offset* of GICR_TYPER (0x0008) 
>>> and
>>> @addr is unlikely to be equal to last_rdist_typer, which is the *GPA* 
>>> of
>>> the last RD. Looks like the user-reading of GICR_TYPER.Last is always
>>> broken?
>> 
>> I think you are right. Somehow, we don't seem to track the index of
>> the RD in the region, so we can never compute the address of the RD
>> even if the base address is set.
>> 
>> Let's drop the reporting of Last for userspace for now, as it never
>> worked. If you post a patch addressing that quickly, I'll get it to
>> Paolo by the end of the week (there's another fix that needs merging).
>> 
>> Eric: do we have any test covering the userspace API?
> 
> So as this issue seems related to the changes made when implementing 
> the
> multiple RDIST regions, I volunteer to write those KVM selftests :-)

You're on! :D

More seriously, there is scope for fuzzing the device save/restore API,
as we find bugs every time someone change the "known good" ordering that
is implemented in QEMU.

Maybe it means getting rid of some unnecessary flexibility, as proposed
by Zenghui, if we are confident that no userspace makes use of it.
And in the future, making sure that new APIs are rigid enough to avoid 
such
bugs.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses
  2020-11-17  8:49           ` Marc Zyngier
  2020-11-17  9:47             ` Auger Eric
  2020-11-17  9:59             ` Auger Eric
@ 2020-11-17 13:09             ` Zenghui Yu
  2 siblings, 0 replies; 12+ messages in thread
From: Zenghui Yu @ 2020-11-17 13:09 UTC (permalink / raw)
  To: Marc Zyngier; +Cc: linux-kernel, linux-arm-kernel, kvmarm

On 2020/11/17 16:49, Marc Zyngier wrote:
> Hi Zenghui,
> 
> On 2020-11-16 14:57, Zenghui Yu wrote:
>> Hi Marc,
>>
>> On 2020/11/16 22:10, Marc Zyngier wrote:
>>>> My take is that only if the "[Re]Distributor base address" is specified
>>>> in the system memory map, will the user-provided kvm_device_attr.offset
>>>> make sense. And we can then handle the access to the register which is
>>>> defined by "base address + offset".
>>>
>>> I'd tend to agree, but it is just that this is a large change at -rc4.
>>> I'd rather have a quick fix for 5.10, and a more invasive change for 
>>> 5.11,
>>> spanning all the possible vgic devices.
>>
>> So you prefer fixing it by "return a value that doesn't have the Last
>> bit set" for v5.10? I'm ok with it and can send v2 for it.
> 
> Cool. Thanks for that.
> 
>> Btw, looking again at the way we handle the user-reading of GICR_TYPER
>>
>>     vgic_mmio_read_v3r_typer(vcpu, addr, len)
>>
>> it seems that @addr is actually the *offset* of GICR_TYPER (0x0008) and
>> @addr is unlikely to be equal to last_rdist_typer, which is the *GPA* of
>> the last RD. Looks like the user-reading of GICR_TYPER.Last is always
>> broken?
> 
> I think you are right. Somehow, we don't seem to track the index of
> the RD in the region, so we can never compute the address of the RD
> even if the base address is set.
> 
> Let's drop the reporting of Last for userspace for now, as it never
> worked. If you post a patch addressing that quickly, I'll get it to
> Paolo by the end of the week (there's another fix that needs merging).

OK. I'll fix it by providing a uaccess_read callback for GICR_TYPER.


Thanks,
Zenghui

> 
> Eric: do we have any test covering the userspace API?
> 
> Thanks,
> 
>          M.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-11-17 13:09 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-13 14:27 [PATCH 0/2] KVM: arm64: vgic: Fix handling of userspace register accesses Zenghui Yu
2020-11-13 14:28 ` [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses Zenghui Yu
2020-11-15 17:04   ` Marc Zyngier
2020-11-16 13:09     ` Zenghui Yu
2020-11-16 14:10       ` Marc Zyngier
2020-11-16 14:57         ` Zenghui Yu
2020-11-17  8:49           ` Marc Zyngier
2020-11-17  9:47             ` Auger Eric
2020-11-17  9:59             ` Auger Eric
2020-11-17 10:51               ` Marc Zyngier
2020-11-17 13:09             ` Zenghui Yu
2020-11-13 14:28 ` [PATCH 2/2] KVM: arm64: vgic: Forbid invalid userspace Distributor accesses Zenghui Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).