Re: [PATCH 1/3] KVM: arm64: vgic-its: Add a data length check in vgic_its_save_*

[Oliver Upton <oliver.upton@linux.dev>]

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.

-- 
Thanks,
Oliver