Date:   Mon, 22 Nov 2021 17:23:26 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     Jason Wang <jasowang@...hat.com>, mst <mst@...hat.com>,
        virtualization <virtualization@...ts.linux-foundation.org>,
        "Hetzelt, Felicitas" <f.hetzelt@...berlin.de>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "kaplan, david" <david.kaplan@....com>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Stefan Hajnoczi <stefanha@...hat.com>
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@...hat.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@...ux.ibm.com> wrote:
>> >>>
>> >>>On Mon, 22 Nov 2021 06:35:18 +0100
>> >>>Halil Pasic <pasic@...ux.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

Powered by blists - more mailing lists