| Message ID | e3f2d7db477734afe5c6a5ab3fa8b8317514ea34.1688746840.git.yin31149@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] vdpa: Fix possible use-after-free for VirtQueueElement | expand |
On Fri, Jul 7, 2023 at 6:44 PM Hawkins Jiawei <yin31149@gmail.com> wrote: > > QEMU uses vhost_handle_guest_kick() to forward guest's available > buffers to the vdpa device in SVQ avail ring. > > In vhost_handle_guest_kick(), a `g_autofree` `elem` is used to > iterate through the available VirtQueueElements. This `elem` is > then passed to `svq->ops->avail_handler`, specifically to the > vhost_vdpa_net_handle_ctrl_avail(). If this handler fails to > process the CVQ command, vhost_handle_guest_kick() regains > ownership of the `elem`, and either frees it or requeues it. > > Yet the problem is that, vhost_vdpa_net_handle_ctrl_avail() > mistakenly frees the `elem`, even if it fails to forward the > CVQ command to vdpa device. This can result in a use-after-free > for the `elem` in vhost_handle_guest_kick(). > > This patch solves this problem by refactoring > vhost_vdpa_net_handle_ctrl_avail() to only freeing the `elem` if > it owns it. > > Fixes: bd907ae4b0 ("vdpa: manual forward CVQ buffers") > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> Reviewed-by: Eugenio Pérez <eperezma@redhat.com> Good catch! > --- > net/vhost-vdpa.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c > index 373609216f..d8f37694ac 100644 > --- a/net/vhost-vdpa.c > +++ b/net/vhost-vdpa.c > @@ -825,7 +825,16 @@ out: > error_report("Bad device CVQ written length"); > } > vhost_svq_push_elem(svq, elem, MIN(in_len, sizeof(status))); > - g_free(elem); > + /* > + * `elem` belongs to vhost_vdpa_net_handle_ctrl_avail() only when > + * the function successfully forwards the CVQ command, indicated > + * by a non-negative value of `dev_written`. Otherwise, it still > + * belongs to SVQ. > + * This function should only free the `elem` when it owns. > + */ > + if (dev_written >= 0) { > + g_free(elem); > + } > return dev_written < 0 ? dev_written : 0; > } > > -- > 2.25.1 >
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 373609216f..d8f37694ac 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -825,7 +825,16 @@ out: error_report("Bad device CVQ written length"); } vhost_svq_push_elem(svq, elem, MIN(in_len, sizeof(status))); - g_free(elem); + /* + * `elem` belongs to vhost_vdpa_net_handle_ctrl_avail() only when + * the function successfully forwards the CVQ command, indicated + * by a non-negative value of `dev_written`. Otherwise, it still + * belongs to SVQ. + * This function should only free the `elem` when it owns. + */ + if (dev_written >= 0) { + g_free(elem); + } return dev_written < 0 ? dev_written : 0; }
QEMU uses vhost_handle_guest_kick() to forward guest's available buffers to the vdpa device in SVQ avail ring. In vhost_handle_guest_kick(), a `g_autofree` `elem` is used to iterate through the available VirtQueueElements. This `elem` is then passed to `svq->ops->avail_handler`, specifically to the vhost_vdpa_net_handle_ctrl_avail(). If this handler fails to process the CVQ command, vhost_handle_guest_kick() regains ownership of the `elem`, and either frees it or requeues it. Yet the problem is that, vhost_vdpa_net_handle_ctrl_avail() mistakenly frees the `elem`, even if it fails to forward the CVQ command to vdpa device. This can result in a use-after-free for the `elem` in vhost_handle_guest_kick(). This patch solves this problem by refactoring vhost_vdpa_net_handle_ctrl_avail() to only freeing the `elem` if it owns it. Fixes: bd907ae4b0 ("vdpa: manual forward CVQ buffers") Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> --- net/vhost-vdpa.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)