* [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
* 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
* [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
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).