Re: [PATCH] hw/net/virtio-net.c: fix crash in iov_copy()

["Alex Bennée" <alex.bennee@linaro.org>]

Дмитрий Фролов <frolov@swemel.ru> writes:

> ping
>
> https://patchew.org/QEMU/20240527133140.218300-2-frolov@swemel.ru/
>
> On 27.05.2024 16:31, Dmitry Frolov wrote:
>> A crash found while fuzzing device virtio-net-socket-check-used.
>> Assertion "offset == 0" in iov_copy() fails if less than guest_hdr_len bytes
>> were transmited.
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>> ---
>>   hw/net/virtio-net.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 24e5e7d347..603b80a50a 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -2783,6 +2783,12 @@ static int32_t virtio_net_flush_tx(VirtIONetQueue *q)
>>            */
>>           assert(n->host_hdr_len <= n->guest_hdr_len);
>>           if (n->host_hdr_len != n->guest_hdr_len) {
>> +            if (iov_size(out_sg, out_num) < n->guest_hdr_len) {
>> +                virtio_error(vdev, "virtio-net header is invalid");
>> +                virtqueue_detach_element(q->tx_vq, elem, 0);
>> +                g_free(elem);
>> +                return -EINVAL;
>> +            }

Isn't this basically another case for goto detach?

Although the use of goto's here is a bit of a code smell. I wonder if
there is any way to better structure this function and take care of the
auto-freeing of elements?

>>               unsigned sg_num = iov_copy(sg, ARRAY_SIZE(sg),
>>                                          out_sg, out_num,
>>                                          0, n->host_hdr_len);

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro