| Message ID | 20210928184803.2496885-2-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic: Missing checks for REDIST/CPU and ITS regions above the VM IPA size | expand |
Hi Ricardo, On 9/28/21 8:47 PM, Ricardo Koller wrote: > Add the new vgic_check_iorange helper that checks that an iorange is > sane: the start address and size have valid alignments, the range is > within the addressable PA range, start+size doesn't overflow, and the > start wasn't already defined. > > No functional change. > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > --- > arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ > arch/arm64/kvm/vgic/vgic.h | 4 ++++ > 2 files changed, 26 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c > index 7740995de982..f714aded67b2 100644 > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c > @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > return 0; > } > > +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > + phys_addr_t addr, phys_addr_t alignment, > + phys_addr_t size) > +{ > + int ret; > + > + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); nit: not related to this patch but I am just wondering why we are passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to vgic_check_iorange()? This must be a leftover of some old code? > + if (ret) > + return ret; > + > + if (!IS_ALIGNED(size, alignment)) > + return -EINVAL; > + > + if (addr + size < addr) > + return -EINVAL; > + > + if (addr + size > kvm_phys_size(kvm)) > + return -E2BIG; > + > + return 0; > +} > + > static int vgic_check_type(struct kvm *kvm, int type_needed) > { > if (kvm->arch.vgic.vgic_model != type_needed) > diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h > index 14a9218641f5..c4df4dcef31f 100644 > --- a/arch/arm64/kvm/vgic/vgic.h > +++ b/arch/arm64/kvm/vgic/vgic.h > @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq, > int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > phys_addr_t addr, phys_addr_t alignment); > > +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > + phys_addr_t addr, phys_addr_t alignment, > + phys_addr_t size); > + > void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu); > void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr); > void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr); Besides Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote: > Hi Ricardo, > > On 9/28/21 8:47 PM, Ricardo Koller wrote: > > Add the new vgic_check_iorange helper that checks that an iorange is > > sane: the start address and size have valid alignments, the range is > > within the addressable PA range, start+size doesn't overflow, and the > > start wasn't already defined. > > > > No functional change. > > > > Signed-off-by: Ricardo Koller <ricarkol@google.com> > > --- > > arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ > > arch/arm64/kvm/vgic/vgic.h | 4 ++++ > > 2 files changed, 26 insertions(+) > > > > diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c > > index 7740995de982..f714aded67b2 100644 > > --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c > > +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c > > @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > > return 0; > > } > > > > +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > > + phys_addr_t addr, phys_addr_t alignment, > > + phys_addr_t size) > > +{ > > + int ret; > > + > > + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); > nit: not related to this patch but I am just wondering why we are > passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to > > vgic_check_iorange()? This must be a leftover of some old code? > It's used to check that the base of a region is not already set. kvm_vgic_addr() uses it to make that check; vgic_v3_alloc_redist_region() does not: rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); Thanks, Ricardo > > + if (ret) > > + return ret; > > + > > + if (!IS_ALIGNED(size, alignment)) > > + return -EINVAL; > > + > > + if (addr + size < addr) > > + return -EINVAL; > > + > > + if (addr + size > kvm_phys_size(kvm)) > > + return -E2BIG; > > + > > + return 0; > > +} > > + > > static int vgic_check_type(struct kvm *kvm, int type_needed) > > { > > if (kvm->arch.vgic.vgic_model != type_needed) > > diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h > > index 14a9218641f5..c4df4dcef31f 100644 > > --- a/arch/arm64/kvm/vgic/vgic.h > > +++ b/arch/arm64/kvm/vgic/vgic.h > > @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq, > > int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > > phys_addr_t addr, phys_addr_t alignment); > > > > +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > > + phys_addr_t addr, phys_addr_t alignment, > > + phys_addr_t size); > > + > > void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu); > > void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr); > > void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr); > Besides > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Eric >
Hi, On 9/29/21 11:17 PM, Ricardo Koller wrote: > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote: >> Hi Ricardo, >> >> On 9/28/21 8:47 PM, Ricardo Koller wrote: >>> Add the new vgic_check_iorange helper that checks that an iorange is >>> sane: the start address and size have valid alignments, the range is >>> within the addressable PA range, start+size doesn't overflow, and the >>> start wasn't already defined. >>> >>> No functional change. >>> >>> Signed-off-by: Ricardo Koller <ricarkol@google.com> >>> --- >>> arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ >>> arch/arm64/kvm/vgic/vgic.h | 4 ++++ >>> 2 files changed, 26 insertions(+) >>> >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c >>> index 7740995de982..f714aded67b2 100644 >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, >>> return 0; >>> } >>> >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, >>> + phys_addr_t addr, phys_addr_t alignment, >>> + phys_addr_t size) >>> +{ >>> + int ret; >>> + >>> + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); >> nit: not related to this patch but I am just wondering why we are >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to >> >> vgic_check_iorange()? This must be a leftover of some old code? >> > It's used to check that the base of a region is not already set. > kvm_vgic_addr() uses it to make that check; > vgic_v3_alloc_redist_region() does not: > > rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes > ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); Yes but I meant why a pointer? Eric > > Thanks, > Ricardo > >>> + if (ret) >>> + return ret; >>> + >>> + if (!IS_ALIGNED(size, alignment)) >>> + return -EINVAL; >>> + >>> + if (addr + size < addr) >>> + return -EINVAL; >>> + >>> + if (addr + size > kvm_phys_size(kvm)) >>> + return -E2BIG; >>> + >>> + return 0; >>> +} >>> + >>> static int vgic_check_type(struct kvm *kvm, int type_needed) >>> { >>> if (kvm->arch.vgic.vgic_model != type_needed) >>> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h >>> index 14a9218641f5..c4df4dcef31f 100644 >>> --- a/arch/arm64/kvm/vgic/vgic.h >>> +++ b/arch/arm64/kvm/vgic/vgic.h >>> @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq, >>> int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, >>> phys_addr_t addr, phys_addr_t alignment); >>> >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, >>> + phys_addr_t addr, phys_addr_t alignment, >>> + phys_addr_t size); >>> + >>> void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu); >>> void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr); >>> void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr); >> Besides >> Reviewed-by: Eric Auger <eric.auger@redhat.com> >> Eric >>
On Thu, Sep 30, 2021 at 09:02:12AM +0200, Eric Auger wrote: > Hi, > > On 9/29/21 11:17 PM, Ricardo Koller wrote: > > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote: > >> Hi Ricardo, > >> > >> On 9/28/21 8:47 PM, Ricardo Koller wrote: > >>> Add the new vgic_check_iorange helper that checks that an iorange is > >>> sane: the start address and size have valid alignments, the range is > >>> within the addressable PA range, start+size doesn't overflow, and the > >>> start wasn't already defined. > >>> > >>> No functional change. > >>> > >>> Signed-off-by: Ricardo Koller <ricarkol@google.com> > >>> --- > >>> arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ > >>> arch/arm64/kvm/vgic/vgic.h | 4 ++++ > >>> 2 files changed, 26 insertions(+) > >>> > >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c > >>> index 7740995de982..f714aded67b2 100644 > >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c > >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c > >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > >>> return 0; > >>> } > >>> > >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > >>> + phys_addr_t addr, phys_addr_t alignment, > >>> + phys_addr_t size) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); > >> nit: not related to this patch but I am just wondering why we are > >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to > >> > >> vgic_check_iorange()? This must be a leftover of some old code? > >> > > It's used to check that the base of a region is not already set. > > kvm_vgic_addr() uses it to make that check; > > vgic_v3_alloc_redist_region() does not: > > > > rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes > > ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > Yes but I meant why a pointer? I can't think of any good reason. It must be some leftover as you said. > > Eric > > > > Thanks, > > Ricardo > > > >>> + if (ret) > >>> + return ret; > >>> + > >>> + if (!IS_ALIGNED(size, alignment)) > >>> + return -EINVAL; > >>> + > >>> + if (addr + size < addr) > >>> + return -EINVAL; > >>> + > >>> + if (addr + size > kvm_phys_size(kvm)) > >>> + return -E2BIG; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int vgic_check_type(struct kvm *kvm, int type_needed) > >>> { > >>> if (kvm->arch.vgic.vgic_model != type_needed) > >>> diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h > >>> index 14a9218641f5..c4df4dcef31f 100644 > >>> --- a/arch/arm64/kvm/vgic/vgic.h > >>> +++ b/arch/arm64/kvm/vgic/vgic.h > >>> @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq, > >>> int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > >>> phys_addr_t addr, phys_addr_t alignment); > >>> > >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > >>> + phys_addr_t addr, phys_addr_t alignment, > >>> + phys_addr_t size); > >>> + > >>> void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu); > >>> void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr); > >>> void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr); > >> Besides > >> Reviewed-by: Eric Auger <eric.auger@redhat.com> > >> Eric > >> >
On Thu, 30 Sep 2021 22:19:16 +0100, Ricardo Koller <ricarkol@google.com> wrote: > > On Thu, Sep 30, 2021 at 09:02:12AM +0200, Eric Auger wrote: > > Hi, > > > > On 9/29/21 11:17 PM, Ricardo Koller wrote: > > > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote: > > >> Hi Ricardo, > > >> > > >> On 9/28/21 8:47 PM, Ricardo Koller wrote: > > >>> Add the new vgic_check_iorange helper that checks that an iorange is > > >>> sane: the start address and size have valid alignments, the range is > > >>> within the addressable PA range, start+size doesn't overflow, and the > > >>> start wasn't already defined. > > >>> > > >>> No functional change. > > >>> > > >>> Signed-off-by: Ricardo Koller <ricarkol@google.com> > > >>> --- > > >>> arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ > > >>> arch/arm64/kvm/vgic/vgic.h | 4 ++++ > > >>> 2 files changed, 26 insertions(+) > > >>> > > >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c > > >>> index 7740995de982..f714aded67b2 100644 > > >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c > > >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c > > >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > > >>> return 0; > > >>> } > > >>> > > >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > > >>> + phys_addr_t addr, phys_addr_t alignment, > > >>> + phys_addr_t size) > > >>> +{ > > >>> + int ret; > > >>> + > > >>> + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); > > >> nit: not related to this patch but I am just wondering why we are > > >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to > > >> > > >> vgic_check_iorange()? This must be a leftover of some old code? > > >> > > > It's used to check that the base of a region is not already set. > > > kvm_vgic_addr() uses it to make that check; > > > vgic_v3_alloc_redist_region() does not: > > > > > > rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes > > > ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > > Yes but I meant why a pointer? > > I can't think of any good reason. It must be some leftover as you said. It definitely is. Please have a patch to fix that. Also, it doesn't look like vgic_check_ioaddr() has any other user at the end of the series. Worth getting rid of. M.
On Fri, Oct 01, 2021 at 02:12:17PM +0100, Marc Zyngier wrote: > On Thu, 30 Sep 2021 22:19:16 +0100, > Ricardo Koller <ricarkol@google.com> wrote: > > > > On Thu, Sep 30, 2021 at 09:02:12AM +0200, Eric Auger wrote: > > > Hi, > > > > > > On 9/29/21 11:17 PM, Ricardo Koller wrote: > > > > On Wed, Sep 29, 2021 at 06:29:21PM +0200, Eric Auger wrote: > > > >> Hi Ricardo, > > > >> > > > >> On 9/28/21 8:47 PM, Ricardo Koller wrote: > > > >>> Add the new vgic_check_iorange helper that checks that an iorange is > > > >>> sane: the start address and size have valid alignments, the range is > > > >>> within the addressable PA range, start+size doesn't overflow, and the > > > >>> start wasn't already defined. > > > >>> > > > >>> No functional change. > > > >>> > > > >>> Signed-off-by: Ricardo Koller <ricarkol@google.com> > > > >>> --- > > > >>> arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ > > > >>> arch/arm64/kvm/vgic/vgic.h | 4 ++++ > > > >>> 2 files changed, 26 insertions(+) > > > >>> > > > >>> diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c > > > >>> index 7740995de982..f714aded67b2 100644 > > > >>> --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c > > > >>> +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c > > > >>> @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, > > > >>> return 0; > > > >>> } > > > >>> > > > >>> +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, > > > >>> + phys_addr_t addr, phys_addr_t alignment, > > > >>> + phys_addr_t size) > > > >>> +{ > > > >>> + int ret; > > > >>> + > > > >>> + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); > > > >> nit: not related to this patch but I am just wondering why we are > > > >> passing phys_addr_t *ioaddr downto vgic_check_ioaddr and thus to > > > >> > > > >> vgic_check_iorange()? This must be a leftover of some old code? > > > >> > > > > It's used to check that the base of a region is not already set. > > > > kvm_vgic_addr() uses it to make that check; > > > > vgic_v3_alloc_redist_region() does not: > > > > > > > > rdreg->base = VGIC_ADDR_UNDEF; // so the "not already defined" check passes > > > > ret = vgic_check_ioaddr(kvm, &rdreg->base, base, SZ_64K); > > > Yes but I meant why a pointer? > > > > I can't think of any good reason. It must be some leftover as you said. > > It definitely is. Please have a patch to fix that. Also, it doesn't > look like vgic_check_ioaddr() has any other user at the end of the > series. Worth getting rid of. ACK fixing that and getting rid of vgic_check_ioaddr(). Thanks, Ricardo > > M. > > -- > Without deviation from the norm, progress is not possible.
diff --git a/arch/arm64/kvm/vgic/vgic-kvm-device.c b/arch/arm64/kvm/vgic/vgic-kvm-device.c index 7740995de982..f714aded67b2 100644 --- a/arch/arm64/kvm/vgic/vgic-kvm-device.c +++ b/arch/arm64/kvm/vgic/vgic-kvm-device.c @@ -29,6 +29,28 @@ int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, return 0; } +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, + phys_addr_t addr, phys_addr_t alignment, + phys_addr_t size) +{ + int ret; + + ret = vgic_check_ioaddr(kvm, ioaddr, addr, alignment); + if (ret) + return ret; + + if (!IS_ALIGNED(size, alignment)) + return -EINVAL; + + if (addr + size < addr) + return -EINVAL; + + if (addr + size > kvm_phys_size(kvm)) + return -E2BIG; + + return 0; +} + static int vgic_check_type(struct kvm *kvm, int type_needed) { if (kvm->arch.vgic.vgic_model != type_needed) diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h index 14a9218641f5..c4df4dcef31f 100644 --- a/arch/arm64/kvm/vgic/vgic.h +++ b/arch/arm64/kvm/vgic/vgic.h @@ -175,6 +175,10 @@ void vgic_irq_handle_resampling(struct vgic_irq *irq, int vgic_check_ioaddr(struct kvm *kvm, phys_addr_t *ioaddr, phys_addr_t addr, phys_addr_t alignment); +int vgic_check_iorange(struct kvm *kvm, phys_addr_t *ioaddr, + phys_addr_t addr, phys_addr_t alignment, + phys_addr_t size); + void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu); void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr); void vgic_v2_clear_lr(struct kvm_vcpu *vcpu, int lr);
Add the new vgic_check_iorange helper that checks that an iorange is sane: the start address and size have valid alignments, the range is within the addressable PA range, start+size doesn't overflow, and the start wasn't already defined. No functional change. Signed-off-by: Ricardo Koller <ricarkol@google.com> --- arch/arm64/kvm/vgic/vgic-kvm-device.c | 22 ++++++++++++++++++++++ arch/arm64/kvm/vgic/vgic.h | 4 ++++ 2 files changed, 26 insertions(+)