On Thu, 12 Sep 2024 13:23:58 +0200 Laurent Vivier <lvivier(a)redhat.com> wrote:On 10/09/2024 17:47, Stefano Brivio wrote:Right, I see that.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.+ +/** + * 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? --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