On Wed, Feb 03, 2021 at 04:45:51PM +0000, Stefan Hajnoczi wrote:

On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:

quoted

On Tue, Feb 02, 2021 at 09:34:12AM +0000, Stefan Hajnoczi wrote:

quoted

On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:

quoted

+static void vdpasim_blk_work(struct work_struct *work)
+{
+	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+	u8 status = VIRTIO_BLK_S_OK;
+	int i;
+
+	spin_lock(&vdpasim->lock);
+
+	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+		goto out;
+
+	for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+		struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+
+		if (!vq->ready)
+			continue;
+
+		while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
+					    &vq->in_iov, &vq->head,
+					    GFP_ATOMIC) > 0) {
+			int write;
+
+			vq->in_iov.i = vq->in_iov.used - 1;
+			write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
+						      &status, 1);
+			if (write <= 0)
+				break;

This code looks fragile:

1. Relying on unsigned underflow and the while loop in
  vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
  risky and could break.

2. Does this assume that the last in_iov element has size 1? For
  example, the guest driver may send a single "in" iovec with size 513
  when reading 512 bytes (with an extra byte for the request status).

Please validate inputs fully, even in test/development code, because
it's likely to be copied by others when writing production code (or
deployed in production by unsuspecting users) :).

Perfectly agree on that, so I addressed these things, also following your
review on the previous version, on the next patch of this series:
"vdpa_sim_blk: implement ramdisk behaviour".

Do you think should I move these checks in this patch?

I did this to leave Max credit for this patch and add more code to emulate a
ramdisk in later patches.

You could update the commit description so it's clear that input
validation is missing and will be added in the next commit.

I'll do it.

Thanks,
Stefano

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization