Re: [PATCH v4 2/4] vhost-user: introduce virtio API

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 12 Sep 2024 13:23:58 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> On 10/09/2024 17:47, Stefano Brivio wrote:
> >> +
> >> +/**
> >> + * virtqueue_read_indirect_desc() - Copy virtio ring descriptors from guest
> >> + *                                  memory
> >> + * @dev:	Vhost-user device
> >> + * @desc:	Destination address to copy the descriptors to
> >> + * @addr:	Guest memory address to copy from
> >> + * @len:	Length of memory to copy
> >> + *
> >> + * Return: -1 if there is an error, 0 otherwise
> >> + */
> >> +static int virtqueue_read_indirect_desc(struct vu_dev *dev, struct vring_desc *desc,
> >> +					uint64_t addr, size_t len)
> >> +{
> >> +	uint64_t read_len;
> >> +
> >> +	if (len > (VIRTQUEUE_MAX_SIZE * sizeof(struct vring_desc)))
> >> +		return -1;
> >> +
> >> +	if (len == 0)
> >> +		return -1;
> >> +
> >> +	while (len) {
> >> +		const struct vring_desc *orig_desc;
> >> +
> >> +		read_len = len;
> >> +		orig_desc = vu_gpa_to_va(dev, &read_len, addr);  
> > In case you missed this in my review of v3 (I'm not sure if it's a
> > valid concern):
> > 
> > --
> > Should we also return if read_len < sizeof(struct vring_desc) after
> > this call? Can that ever happen, if we pick a particular value of addr
> > so that it's almost at the end of a region?
> > --  
> 
> In fact, read_len can be < sizeof(struct vring_desc) after this call but if orig_desc != 
> NULL it means we can continue in another region to continue to fill the structure.

Right, I see that.

> If there is not enough memory to fill "len" bytes it exits with -1.

...and this as well. But let's say that read_len is 1 (and struct
vring_desc is 16 bytes). Then:

		memcpy(desc, orig_desc, read_len);

copies one byte

		[...]

		desc += read_len / sizeof(struct vring_desc);

doesn't increase desc.

At the next iteration with len > 0 and read_len > 0, the memcpy() will
overwrite that one byte, as we didn't increase desc. Or it's not
possible for some other reason?

-- 
Stefano