Nikolay Kuratov <[email protected]> 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: [email protected]
> Signed-off-by: Nikolay Kuratov <[email protected]>
> ---
> 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' } }
