Re: [PATCH V5 1/4] virtio_ring: validate used buffer length

[Stefano Garzarella]

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@xxxxxxxxxxxxx> wrote:
> > > On Mon, 22 Nov 2021 06:35:18 +0100
> > > Halil Pasic <pasic@xxxxxxxxxxxxx> 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.

Stefano