[PATCH] qmp: Check vhost protocol features for NULL prior to dumping

[PATCH] qmp: Check vhost protocol features for NULL prior to dumping

[Nikolay Kuratov]

vhost_dev->protocol_features field can be legitimately set to NULL
as apparently it's vhost-user only field. At least I was able to get
NULL deref with a vhost-net VM.

Without that check querying vhost-net device:
info virtio-status /machine/peripheral-anon/device[0]/virtio-backend
will lead to SIGSEGV.

Fixes: 8a8287981d1169f534894d983ecfd3b70b71918b ("hmp: add virtio commands")
Cc: qemu-stable@nongnu.org
Signed-off-by: Nikolay Kuratov <kniv@yandex-team.ru>
---
 hw/virtio/virtio-hmp-cmds.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
index 1daae482d3..b9980197ef 100644
--- a/hw/virtio/virtio-hmp-cmds.c
+++ b/hw/virtio/virtio-hmp-cmds.c
@@ -15,6 +15,9 @@
 static void hmp_virtio_dump_protocols(Monitor *mon,
                                       VhostDeviceProtocols *pcol)
 {
+    if (pcol == NULL) {
+        return;
+    }
     strList *pcol_list = pcol->protocols;
     while (pcol_list) {
         monitor_printf(mon, "\t%s", pcol_list->value);
-- 
2.34.1

Re: [PATCH] qmp: Check vhost protocol features for NULL prior to dumping

[Markus Armbruster]

Nikolay Kuratov <kniv@yandex-team.ru> writes:

> vhost_dev->protocol_features field can be legitimately set to NULL
> as apparently it's vhost-user only field. At least I was able to get
> NULL deref with a vhost-net VM.
>
> Without that check querying vhost-net device:
> info virtio-status /machine/peripheral-anon/device[0]/virtio-backend
> will lead to SIGSEGV.
>
> Fixes: 8a8287981d1169f534894d983ecfd3b70b71918b ("hmp: add virtio commands")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Nikolay Kuratov <kniv@yandex-team.ru>
> ---
>  hw/virtio/virtio-hmp-cmds.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/virtio/virtio-hmp-cmds.c b/hw/virtio/virtio-hmp-cmds.c
> index 1daae482d3..b9980197ef 100644
> --- a/hw/virtio/virtio-hmp-cmds.c
> +++ b/hw/virtio/virtio-hmp-cmds.c
> @@ -15,6 +15,9 @@
>  static void hmp_virtio_dump_protocols(Monitor *mon,
>                                        VhostDeviceProtocols *pcol)
>  {
> +    if (pcol == NULL) {
> +        return;
> +    }
>      strList *pcol_list = pcol->protocols;
>      while (pcol_list) {
>          monitor_printf(mon, "\t%s", pcol_list->value);

I fear this papers over the real bug.

Here's the only caller:

    void hmp_virtio_status(Monitor *mon, const QDict *qdict)
    {
        Error *err = NULL;
        const char *path = qdict_get_try_str(qdict, "path");
        VirtioStatus *s = qmp_x_query_virtio_status(path, &err);

        if (err != NULL) {

Aside: I'd prefer if (!s) here.

            hmp_handle_error(mon, err);
            return;
        }

        [...]

        if (s->vhost_dev) {
            [...]
            monitor_printf(mon, "    Protocol features:\n");
            hmp_virtio_dump_protocols(mon, s->vhost_dev->protocol_features);

We're passing VirtioStatus member @protocol-features.  It's not supposed
to be null, because ...

        }

        qapi_free_VirtioStatus(s);
    }

In qapi/virtio.json:

    { 'struct': 'VhostStatus',
      'data': { 'n-mem-sections': 'int',
                'n-tmp-sections': 'int',
                'nvqs': 'uint32',
                'vq-index': 'int',
                'features': 'VirtioDeviceFeatures',
                'acked-features': 'VirtioDeviceFeatures',
                'backend-features': 'VirtioDeviceFeatures',
                'protocol-features': 'VhostDeviceProtocols',

... member @protocol-features is *not* optional.

Since you obseved it to be null, whatever created s->vhost_dev must have
screwed up.

If s->vhost_dev->protocol-features needs to be null in certain
situations, you must declare it optional in the QAPI schema.

                'max-queues': 'uint64',
                'backend-cap': 'uint64',
                'log-enabled': 'bool',
                'log-size': 'uint64' } }

Re:[PATCH] qmp: Check vhost protocol features for NULL prior to dumping

[Nikolay Kuratov]

> I fear this papers over the real bug.

Sorry, my bad. There is no such bug in upstream since
qmp_x_query_virtio_status() clearly did its job of setting
protocol_features well. So patch should be dropped.

Thank you for pointing to qapi part.