Messages in this thread | | | Date | Mon, 22 Nov 2021 17:23:26 +0100 | From | Stefano Garzarella <> | Subject | Re: [PATCH V5 1/4] virtio_ring: validate used buffer length |
| |
On Mon, Nov 22, 2021 at 03:24:32PM +0100, Halil Pasic wrote: >On Mon, 22 Nov 2021 12:08:22 +0100 >Stefano Garzarella <sgarzare@redhat.com> wrote: > >> On Mon, Nov 22, 2021 at 08:55:24AM +0100, Stefano Garzarella wrote: >> >On Mon, Nov 22, 2021 at 02:25:26PM +0800, Jason Wang wrote: >> >>On Mon, Nov 22, 2021 at 1:49 PM Halil Pasic <pasic@linux.ibm.com> wrote: >> >>> >> >>>On Mon, 22 Nov 2021 06:35:18 +0100 >> >>>Halil Pasic <pasic@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 >> > >> >Yep, in vhost_vsock_handle_tx_kick() we should have vhost_add_used(vq, >> >head, 0) since the device doesn't write anything. >> > >> >>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. >> > >> >I fully agree with these steps. >> >> Michael sent a patch to suppress the validation, so I think we should >> just fix vhost-vsock. I mean something like this: >> >> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c >> index 938aefbc75ec..4e3b95af7ee4 100644 >> --- a/drivers/vhost/vsock.c >> +++ b/drivers/vhost/vsock.c >> @@ -554,7 +554,7 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work *work) >> virtio_transport_free_pkt(pkt); >> >> len += sizeof(pkt->hdr); >> - vhost_add_used(vq, head, len); >> + vhost_add_used(vq, head, 0); >> total_len += len; >> added = true; >> } while(likely(!vhost_exceeds_weight(vq, ++pkts, total_len))); >> >> I checked and the problem is there from the first commit, so we should >> add: >> >> Fixes: 433fc58e6bf2 ("VSOCK: Introduce vhost_vsock.ko") >> >> I tested this patch and it works even without suppressing validation in >> the virtio core. But for backwards compatibility we have to suppress it >> for sure as Michael did. >> >> Maybe we can have a patch just with this change to backport it easily >> and one after to clean up a bit the code that was added after (len, >> total_len). >> >> @Halil Let me know if you want to do it, otherwise I can do it. >> > >It is fine, it was you guys who figured out the solution so I think >it should either be Jason or you who take credit for the patch.
Okay, I'm finishing the tests and sending the patch.
>Thanks for addressing the issue this quickly!
Thanks for reporting!
Stefano
| |