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

[Michael Tokarev]

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.

Thanks,

/mjt