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]

Date:   Mon, 22 Nov 2021 08:55:24 +0100
From:   Stefano Garzarella <sgarzare@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     Halil Pasic <pasic@...ux.ibm.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 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.

Thanks,
Stefano