From: Halil Pasic <pasic@linux.ibm.com> To: Jason Wang <jasowang@redhat.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>, Halil Pasic <pasic@linux.ibm.com> Subject: Re: [PATCH V5 1/4] virtio_ring: validate used buffer length Date: Mon, 22 Nov 2021 06:49:22 +0100 [thread overview] Message-ID: <20211122064922.51b3678e.pasic@linux.ibm.com> (raw) In-Reply-To: <20211122063518.37929c01.pasic@linux.ibm.com> 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. > > 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
WARNING: multiple messages have this Message-ID (diff)
From: Halil Pasic <pasic@linux.ibm.com> To: Jason Wang <jasowang@redhat.com> Cc: "kaplan, david" <david.kaplan@amd.com>, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>, "Hetzelt, Felicitas" <f.hetzelt@tu-berlin.de>, linux-kernel <linux-kernel@vger.kernel.org>, virtualization <virtualization@lists.linux-foundation.org>, Halil Pasic <pasic@linux.ibm.com>, mst <mst@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com> Subject: Re: [PATCH V5 1/4] virtio_ring: validate used buffer length Date: Mon, 22 Nov 2021 06:49:22 +0100 [thread overview] Message-ID: <20211122064922.51b3678e.pasic@linux.ibm.com> (raw) In-Reply-To: <20211122063518.37929c01.pasic@linux.ibm.com> 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. > > 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 _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-11-22 5:49 UTC|newest] Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-27 2:21 [PATCH V5 0/4] Validate used buffer length Jason Wang 2021-10-27 2:21 ` Jason Wang 2021-10-27 2:21 ` [PATCH V5 1/4] virtio_ring: validate " Jason Wang 2021-10-27 2:21 ` Jason Wang 2021-11-02 3:18 ` Xuan Zhuo 2021-11-02 3:54 ` Jason Wang 2021-11-02 3:54 ` Jason Wang 2021-11-19 15:09 ` Halil Pasic 2021-11-19 15:09 ` Halil Pasic 2021-11-22 3:51 ` Jason Wang 2021-11-22 3:51 ` Jason Wang 2021-11-22 5:35 ` Halil Pasic 2021-11-22 5:35 ` Halil Pasic 2021-11-22 5:49 ` Halil Pasic [this message] 2021-11-22 5:49 ` Halil Pasic 2021-11-22 6:25 ` Jason Wang 2021-11-22 6:25 ` Jason Wang 2021-11-22 7:55 ` Stefano Garzarella 2021-11-22 7:55 ` Stefano Garzarella 2021-11-22 11:08 ` Stefano Garzarella 2021-11-22 11:08 ` Stefano Garzarella 2021-11-22 14:24 ` Halil Pasic 2021-11-22 14:24 ` Halil Pasic 2021-11-22 16:23 ` Stefano Garzarella 2021-11-22 16:23 ` Stefano Garzarella 2021-11-22 13:50 ` Halil Pasic 2021-11-22 13:50 ` Halil Pasic 2021-11-23 2:30 ` Jason Wang 2021-11-23 2:30 ` Jason Wang 2021-11-23 12:17 ` Michael S. Tsirkin 2021-11-23 12:17 ` Michael S. Tsirkin 2021-11-23 12:43 ` Halil Pasic 2021-11-23 12:43 ` Halil Pasic 2021-11-22 20:23 ` Halil Pasic 2021-11-22 20:23 ` Halil Pasic 2021-11-23 2:25 ` Jason Wang 2021-11-23 2:25 ` Jason Wang 2021-11-23 11:05 ` Michael S. Tsirkin 2021-11-23 11:05 ` Michael S. Tsirkin 2021-11-24 1:30 ` Michael Ellerman 2021-11-24 1:30 ` Michael Ellerman 2021-11-24 2:26 ` Jason Wang 2021-11-24 2:26 ` Jason Wang 2021-11-24 2:33 ` Jason Wang 2021-11-24 2:33 ` Jason Wang 2021-11-24 7:22 ` Michael S. Tsirkin 2021-11-24 7:22 ` Michael S. Tsirkin 2021-11-24 7:59 ` Jason Wang 2021-11-24 7:59 ` Jason Wang 2021-11-24 8:24 ` Michael S. Tsirkin 2021-11-24 8:24 ` Michael S. Tsirkin 2021-11-24 8:28 ` Jason Wang 2021-11-24 8:28 ` Jason Wang 2021-11-24 11:33 ` Halil Pasic 2021-11-24 11:33 ` Halil Pasic 2021-11-25 2:27 ` Jason Wang 2021-11-25 2:27 ` Jason Wang 2021-11-22 7:42 ` Stefano Garzarella 2021-11-22 7:42 ` Stefano Garzarella 2021-10-27 2:21 ` [PATCH V5 2/4] virtio-net: don't let virtio core to validate used length Jason Wang 2021-10-27 2:21 ` Jason Wang 2021-10-27 2:21 ` [PATCH V5 3/4] virtio-blk: " Jason Wang 2021-10-27 2:21 ` Jason Wang 2021-10-27 2:21 ` [PATCH V5 4/4] virtio-scsi: don't let virtio core to validate used buffer length Jason Wang 2021-10-27 2:21 ` Jason Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211122064922.51b3678e.pasic@linux.ibm.com \ --to=pasic@linux.ibm.com \ --cc=david.kaplan@amd.com \ --cc=f.hetzelt@tu-berlin.de \ --cc=jasowang@redhat.com \ --cc=konrad.wilk@oracle.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.