| Message ID | 3d90d47995b83bd1edf6e756c00e74fd5ec16aee.1687782442.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/53] bswap: Add the ability to store to an unaligned 24 bit field | expand |
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
> 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.
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) {