| Message ID | 20201113142801.1659-2-yuzenghui@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic: Fix handling of userspace register accesses | expand |
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.
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.
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.
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.
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.
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); }
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(+)