MIME-Version: 1.0
References: <20211027022107.14357-1-jasowang@redhat.com> <20211027022107.14357-2-jasowang@redhat.com>
 <20211119160951.5f2294c8.pasic@linux.ibm.com> <CACGkMEtja2TPC=ujgMrpaPmdsy+zHowbBTvPj8k7nm_+zB8vig@mail.gmail.com>
 <20211122063518.37929c01.pasic@linux.ibm.com> <20211122064922.51b3678e.pasic@linux.ibm.com>
In-Reply-To: <20211122064922.51b3678e.pasic@linux.ibm.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Mon, 22 Nov 2021 14:25:26 +0800
Message-ID: <CACGkMEu+9FvMsghyi55Ee5BxetP-YK9wh2oaT8OgLiY5+tV0QQ@mail.gmail.com>
Subject: Re: [PATCH V5 1/4] virtio_ring: validate used buffer length
To:     Halil Pasic <pasic@linux.ibm.com>
Cc:     mst <mst@redhat.com>,
        virtualization <virtualization@lists.linux-foundation.org>,
        "Hetzelt, Felicitas" <f.hetzelt@tu-berlin.de>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "kaplan, david" <david.kaplan@amd.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Stefan Hajnoczi <stefanha@redhat.com>,
        Stefano Garzarella <sgarzare@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
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
>