| Message ID | 20240620130650.1279-2-jiangkunkun@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic-its: Some fixes about vgic-its | expand |
Hi, On Thu, Jun 20, 2024 at 09:06:48PM +0800, Kunkun Jiang wrote: > In all the vgic_its_save_*() functinos, it does not check > whether the data length is larger than 8 bytes before > calling vgic_write_guest_lock. This patch add the check. > > Link: https://lore.kernel.org/kvmarm/86v82ckimh.wl-maz@kernel.org/ > Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> > --- > arch/arm64/kvm/vgic/vgic-its.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c > index 40bb43f20bf3..060605fba3b6 100644 > --- a/arch/arm64/kvm/vgic/vgic-its.c > +++ b/arch/arm64/kvm/vgic/vgic-its.c > @@ -2094,6 +2094,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, > ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) | > ite->collection->collection_id; > val = cpu_to_le64(val); > + BUG_ON(ite_esz > sizeof(val)); Does it really make sense to blow up the kernel over this? (hint: no) What _might_ make sense if if you bugged the VM and failed the ioctl, i.e. if (KVM_BUG_ON(ite_esz != sizeof(val), kvm)) return -EINVAL; Also, this isn't even asserting the right thing. You want to assert that the u64 being written to memory is *exactly* the size of a single ITE. No more, no less. > return vgic_write_guest_lock(kvm, gpa, &val, ite_esz); > } > > @@ -2246,6 +2247,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev, > (itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) | > (dev->num_eventid_bits - 1)); > val = cpu_to_le64(val); > + BUG_ON(dte_esz > sizeof(dte_esz)); Did you even test this? A bit of substitution arrives at: BUG_ON(8 > sizeof(unsigned int)); See the issue? Please do not test these sort of untested patches on the list, it is a waste of everyone's time.
Hi Oliver, On 2024/6/21 6:25, Oliver Upton wrote: > Hi, > > On Thu, Jun 20, 2024 at 09:06:48PM +0800, Kunkun Jiang wrote: >> In all the vgic_its_save_*() functinos, it does not check >> whether the data length is larger than 8 bytes before >> calling vgic_write_guest_lock. This patch add the check. >> >> Link: https://lore.kernel.org/kvmarm/86v82ckimh.wl-maz@kernel.org/ >> Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> >> --- >> arch/arm64/kvm/vgic/vgic-its.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c >> index 40bb43f20bf3..060605fba3b6 100644 >> --- a/arch/arm64/kvm/vgic/vgic-its.c >> +++ b/arch/arm64/kvm/vgic/vgic-its.c >> @@ -2094,6 +2094,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, >> ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) | >> ite->collection->collection_id; >> val = cpu_to_le64(val); >> + BUG_ON(ite_esz > sizeof(val)); > Does it really make sense to blow up the kernel over this? (hint: no) > > What _might_ make sense if if you bugged the VM and failed the ioctl, > i.e. > > if (KVM_BUG_ON(ite_esz != sizeof(val), kvm)) > return -EINVAL; > > Also, this isn't even asserting the right thing. You want to assert that > the u64 being written to memory is *exactly* the size of a single ITE. > No more, no less. This makes sense. I will modify it in the next version. > static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz) > { > struct its_collection *collection; > struct kvm *kvm = its->dev->kvm; > u32 target_addr, coll_id; > u64 val; > int ret; > > BUG_ON(esz > sizeof(val)); > ret = kvm_read_guest_lock(kvm, gpa, &val, esz); It should also be modified synchronously, right? >> return vgic_write_guest_lock(kvm, gpa, &val, ite_esz); >> } >> >> @@ -2246,6 +2247,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev, >> (itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) | >> (dev->num_eventid_bits - 1)); >> val = cpu_to_le64(val); >> + BUG_ON(dte_esz > sizeof(dte_esz)); > Did you even test this? A bit of substitution arrives at: > > BUG_ON(8 > sizeof(unsigned int)); > > See the issue? > > Please do not test these sort of untested patches on the list, it is a > waste of everyone's time. Sorry, there is a problem here. I will pay attention to it in the future. Thank you for your correction. Thanks, Kunkun Jiang
On Fri, Jun 21, 2024 at 09:43:05AM +0800, Kunkun Jiang wrote: > > static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz) > > { > > struct its_collection *collection; > > struct kvm *kvm = its->dev->kvm; > > u32 target_addr, coll_id; > > u64 val; > > int ret; > > > > BUG_ON(esz > sizeof(val)); > > ret = kvm_read_guest_lock(kvm, gpa, &val, esz); > It should also be modified synchronously, right? Do you mean replacing the other BUG_ON()'s in the ITS code with the pattern I'd recommended earlier? That'd be great if you could do that. > > > return vgic_write_guest_lock(kvm, gpa, &val, ite_esz); > > > } > > > @@ -2246,6 +2247,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev, > > > (itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) | > > > (dev->num_eventid_bits - 1)); > > > val = cpu_to_le64(val); > > > + BUG_ON(dte_esz > sizeof(dte_esz)); > > Did you even test this? A bit of substitution arrives at: > > > > BUG_ON(8 > sizeof(unsigned int)); > > > > See the issue? > > > > Please do not test these sort of untested patches on the list, it is a > > waste of everyone's time. > Sorry, there is a problem here. I will pay attention to it in the future. > Thank you for your correction. Apologies for being firm on this, but outside of RFCs the expectation is that the author test changes before posting to the list. In that spirit, a reproducer for the issue you observe in KVM selftests would be great. We have some minimal support for dealing with an ITS over there now.
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index 40bb43f20bf3..060605fba3b6 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -2094,6 +2094,7 @@ static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev, ((u64)ite->irq->intid << KVM_ITS_ITE_PINTID_SHIFT) | ite->collection->collection_id; val = cpu_to_le64(val); + BUG_ON(ite_esz > sizeof(val)); return vgic_write_guest_lock(kvm, gpa, &val, ite_esz); } @@ -2246,6 +2247,7 @@ static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev, (itt_addr_field << KVM_ITS_DTE_ITTADDR_SHIFT) | (dev->num_eventid_bits - 1)); val = cpu_to_le64(val); + BUG_ON(dte_esz > sizeof(dte_esz)); return vgic_write_guest_lock(kvm, ptr, &val, dte_esz); } @@ -2433,6 +2435,7 @@ static int vgic_its_save_cte(struct vgic_its *its, ((u64)collection->target_addr << KVM_ITS_CTE_RDBASE_SHIFT) | collection->collection_id); val = cpu_to_le64(val); + BUG_ON(esz > sizeof(val)); return vgic_write_guest_lock(its->dev->kvm, gpa, &val, esz); }
In all the vgic_its_save_*() functinos, it does not check whether the data length is larger than 8 bytes before calling vgic_write_guest_lock. This patch add the check. Link: https://lore.kernel.org/kvmarm/86v82ckimh.wl-maz@kernel.org/ Signed-off-by: Kunkun Jiang <jiangkunkun@huawei.com> --- arch/arm64/kvm/vgic/vgic-its.c | 3 +++ 1 file changed, 3 insertions(+)