On Mon, Nov 22, 2021 at 1:49 PM Halil Pasic <pa...@linux.ibm.com> wrote: > > On Mon, 22 Nov 2021 06:35:18 +0100 > Halil Pasic <pa...@linux.ibm.com> wrote: > > > > I think it should be a common issue, looking at > > > vhost_vsock_handle_tx_kick(), it did: > > > > > > len += sizeof(pkt->hdr); > > > vhost_add_used(vq, head, len); > > > > > > which looks like a violation of the spec since it's TX. > > > > I'm not sure the lines above look like a violation of the spec. If you > > examine vhost_vsock_alloc_pkt() I believe that you will agree that: > > len == pkt->len == pkt->hdr.len > > which makes sense since according to the spec both tx and rx messages > > are hdr+payload. And I believe hdr.len is the size of the payload, > > although that does not seem to be properly documented by the spec.
Sorry for being unclear, what I meant is that we probably should use zero here. TX doesn't use in buffer actually. According to the spec, 0 should be the used length: "and len the total of bytes written into the buffer." > > > > On the other hand tx messages are stated to be device read-only (in the > > spec) so if the device writes stuff, that is certainly wrong. > > Yes. > > If that is what happens. > > > > Looking at virtqueue_get_buf_ctx_split() I'm not sure that is what > > happens. My hypothesis is that we just a last descriptor is an 'in' > > type descriptor (i.e. a device writable one). For tx that assumption > > would be wrong. > > > > I will have another look at this today and send a fix patch if my > > suspicion is confirmed. > > If my suspicion is right something like: > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 00f64f2f8b72..efb57898920b 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -764,6 +764,7 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue > *_vq, > struct vring_virtqueue *vq = to_vvq(_vq); > void *ret; > unsigned int i; > + bool has_in; > u16 last_used; > > START_USE(vq); > @@ -787,6 +788,9 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue > *_vq, > vq->split.vring.used->ring[last_used].id); > *len = virtio32_to_cpu(_vq->vdev, > vq->split.vring.used->ring[last_used].len); > + has_in = virtio16_to_cpu(_vq->vdev, > + vq->split.vring.used->ring[last_used].flags) > + & VRING_DESC_F_WRITE; Did you mean vring.desc actually? If yes, it's better not depend on the descriptor ring which can be modified by the device. We've stored the flags in desc_extra[]. > > if (unlikely(i >= vq->split.vring.num)) { > BAD_RING(vq, "id %u out of range\n", i); > @@ -796,7 +800,7 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue > *_vq, > BAD_RING(vq, "id %u is not a head!\n", i); > return NULL; > } > - if (vq->buflen && unlikely(*len > vq->buflen[i])) { > + if (has_in && q->buflen && unlikely(*len > vq->buflen[i])) { > BAD_RING(vq, "used len %d is larger than in buflen %u\n", > *len, vq->buflen[i]); > return NULL; > > would fix the problem for split. I will try that out and let you know > later. I'm not sure I get this, in virtqueue_add_split, the buflen[i] only contains the in buffer length. I think the fixes are: 1) fixing the vhost vsock 2) use suppress_used_validation=true to let vsock driver to validate the in buffer length 3) probably a new feature so the driver can only enable the validation when the feature is enabled. Thanks > > Regards, > Halil > _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization