Re: [PULL 53/53] vhost-vdpa: do not cleanup the vdpa/vhost-net structures if peer nic is present

[Ani Sinha]

> On 26-Jun-2023, at 9:23 PM, Michael Tokarev <mjt@tls.msk.ru> wrote:
> 
> 26.06.2023 15:30, Michael S. Tsirkin wrote:
>> From: Ani Sinha <anisinha@redhat.com>
>> When a peer nic is still attached to the vdpa backend, it is too early to 
>> free
>> up the vhost-net and vdpa structures. If these structures are freed here, 
>> then
>> QEMU crashes when the guest is being shut down. The following call chain
>> would result in an assertion failure since the pointer returned from
>> vhost_vdpa_get_vhost_net() would be NULL:
>> do_vm_stop() -> vm_state_notify() -> virtio_set_status() ->
>> virtio_net_vhost_status() -> get_vhost_net().
>> Therefore, we defer freeing up the structures until at guest shutdown
>> time when qemu_cleanup() calls net_cleanup() which then calls
>> qemu_del_net_client() which would eventually call vhost_vdpa_cleanup()
>> again to free up the structures. This time, the loop in net_cleanup()
>> ensures that vhost_vdpa_cleanup() will be called one last time when
>> all the peer nics are detached and freed.
>> All unit tests pass with this change.
>> CC: imammedo@redhat.com
>> CC: jusual@redhat.com
>> CC: mst@redhat.com
>> Fixes: CVE-2023-3301
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2128929
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> Message-Id: <20230619065209.442185-1-anisinha@redhat.com>
>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  net/vhost-vdpa.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 9e92b3558c..e19ab063fa 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -207,6 +207,14 @@ static void vhost_vdpa_cleanup(NetClientState *nc)
>>  {
>>      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>  +    /*
>> +     * If a peer NIC is attached, do not cleanup anything.
>> +     * Cleanup will happen as a part of qemu_cleanup() -> net_cleanup()
>> +     * when the guest is shutting down.
>> +     */
>> +    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_NIC) {
>> +        return;
>> +    }
>>      munmap(s->cvq_cmd_out_buffer, vhost_vdpa_net_cvq_cmd_page_len());
>>      munmap(s->status, vhost_vdpa_net_cvq_cmd_page_len());
>>      if (s->vhost_net) {
> 
> 
> Given the CVE# attached, is it a -stable material?
> The same change can be applied to 8.0 and even to 7.2, with slight difference
> in context (using qemu_vfree() instead of munmap() for cvq_cmd_out_buffer 
> etc).
> The original bugreport is about qemu 7.1.

Yes I think it can be applied to 7.2 and 8.0. I back ported the patch on 
stable-8.0 and tested it and it seems to fix the issue. I have not tested on 
7.2.