[Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues

[Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues

[Yunjian Wang]

The tx_bh or tx_timer will free in virtio_net_del_queue() function, when
removing virtio-net queues if the guest doesn't support multiqueue. But
it might be still referenced by virtio_net_set_status(), which needs to
be set NULL. And also the tx_waiting needs to be set zero to prevent
virtio_net_set_status() accessing tx_bh or tx_timer.

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 hw/net/virtio-net.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7d091c9..98bd683 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1522,9 +1522,12 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
     if (q->tx_timer) {
         timer_del(q->tx_timer);
         timer_free(q->tx_timer);
+        q->tx_timer = NULL;
     } else {
         qemu_bh_delete(q->tx_bh);
+        q->tx_bh = NULL;
     }
+    q->tx_waiting = 0;
     virtio_del_queue(vdev, index * 2 + 1);
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues

[Jason Wang]

On 2017年04月26日 14:45, Yunjian Wang wrote:
> The tx_bh or tx_timer will free in virtio_net_del_queue() function, when
> removing virtio-net queues if the guest doesn't support multiqueue. But
> it might be still referenced by virtio_net_set_status(), which needs to
> be set NULL. And also the tx_waiting needs to be set zero to prevent
> virtio_net_set_status() accessing tx_bh or tx_timer.
>
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>   hw/net/virtio-net.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 7d091c9..98bd683 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1522,9 +1522,12 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
>       if (q->tx_timer) {
>           timer_del(q->tx_timer);
>           timer_free(q->tx_timer);
> +        q->tx_timer = NULL;
>       } else {
>           qemu_bh_delete(q->tx_bh);
> +        q->tx_bh = NULL;
>       }
> +    q->tx_waiting = 0;
>       virtio_del_queue(vdev, index * 2 + 1);
>   }
>   

Thanks for the patch.

It looks to me that clearing tx_waiting is sufficient or is there any 
other reason that you need set tx_timer/tx_bh to NULL?

Thanks

Re: [Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues

[wangyunjian]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com] 
> Sent: Wednesday, April 26, 2017 5:18 PM
> To: wangyunjian <wangyunjian@huawei.com>; mst@redhat.com; qemu-devel@nongnu.org
> Cc: caihe <caihe@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues
> 
> 
> 
> On 2017年04月26日 14:45, Yunjian Wang wrote:
> > The tx_bh or tx_timer will free in virtio_net_del_queue() function, 
> > when removing virtio-net queues if the guest doesn't support 
> > multiqueue. But it might be still referenced by 
> > virtio_net_set_status(), which needs to be set NULL. And also the 
> > tx_waiting needs to be set zero to prevent
> > virtio_net_set_status() accessing tx_bh or tx_timer.
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> >   hw/net/virtio-net.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 
> > 7d091c9..98bd683 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -1522,9 +1522,12 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
> >       if (q->tx_timer) {
> >           timer_del(q->tx_timer);
> >           timer_free(q->tx_timer);
> > +        q->tx_timer = NULL;
> >       } else {
> >           qemu_bh_delete(q->tx_bh);
> > +        q->tx_bh = NULL;
> >       }
> > +    q->tx_waiting = 0;
> >       virtio_del_queue(vdev, index * 2 + 1);
> >   }
> >   
> 
> Thanks for the patch.
> 
> It looks to me that clearing tx_waiting is sufficient or is there any
> other reason that youneed set tx_timer/tx_bh to NULL?

It's just coding habit to avoid access wild pointer, such as
used-after-free porblem, it hard to locate the code that cause the bug.

Thanks

> 
> Thanks

Re: [Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues

[Jason Wang]

On 2017年04月26日 18:45, wangyunjian wrote:
>> -----Original Message-----
>> From: Jason Wang [mailto:jasowang@redhat.com]
>> Sent: Wednesday, April 26, 2017 5:18 PM
>> To: wangyunjian <wangyunjian@huawei.com>; mst@redhat.com; qemu-devel@nongnu.org
>> Cc: caihe <caihe@huawei.com>
>> Subject: Re: [Qemu-devel] [PATCH] virtio-net: fix wild pointer when remove virtio-net queues
>>
>>
>>
>> On 2017年04月26日 14:45, Yunjian Wang wrote:
>>> The tx_bh or tx_timer will free in virtio_net_del_queue() function,
>>> when removing virtio-net queues if the guest doesn't support
>>> multiqueue. But it might be still referenced by
>>> virtio_net_set_status(), which needs to be set NULL. And also the
>>> tx_waiting needs to be set zero to prevent
>>> virtio_net_set_status() accessing tx_bh or tx_timer.
>>>
>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>>> ---
>>>    hw/net/virtio-net.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
>>> 7d091c9..98bd683 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -1522,9 +1522,12 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
>>>        if (q->tx_timer) {
>>>            timer_del(q->tx_timer);
>>>            timer_free(q->tx_timer);
>>> +        q->tx_timer = NULL;
>>>        } else {
>>>            qemu_bh_delete(q->tx_bh);
>>> +        q->tx_bh = NULL;
>>>        }
>>> +    q->tx_waiting = 0;
>>>        virtio_del_queue(vdev, index * 2 + 1);
>>>    }
>>>    
>> Thanks for the patch.
>>
>> It looks to me that clearing tx_waiting is sufficient or is there any
>> other reason that youneed set tx_timer/tx_bh to NULL?
> It's just coding habit to avoid access wild pointer, such as
> used-after-free porblem, it hard to locate the code that cause the bug.
>
> Thanks

Applied and queued for -stable.

Thanks

>> Thanks