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
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 <[email protected]>
Signed-off-by: Zenghui Yu <[email protected]>
---
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
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 <[email protected]>
---
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
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...
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
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...
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
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...
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.
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.
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...
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.