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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211122064922.51b3678e.pasic@linux.ibm.com>
Date:   Mon, 22 Nov 2021 06:49:22 +0100
From:   Halil Pasic <pasic@...ux.ibm.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     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>,
        Stefano Garzarella <sgarzare@...hat.com>,
        Halil Pasic <pasic@...ux.ibm.com>
Subject: Re: [PATCH V5 1/4] virtio_ring: validate used buffer length

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

Regards,
Halil