On 09/09/2022 14:10, Jiacheng Jiang wrote:

From: jiangjiacheng <jiangjiacheng@huawei.com>

In virNetServerClientNew, client->rx is be assigned by invoking virNetServerClientNew, but isn't freed if client->privateData's initialization failed, which leads to a memory leak. And in virNetServerProgramDispatchCall, the error path doesn't free the memory of dispatcher->arg_filter, which leads to a memory leak.

I think it's memory `arg` points to instead of dispatcher->arg_filter that leaks?

Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/rpc/virnetserverclient.c | 1 + src/rpc/virnetserverprogram.c | 12 +++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index a7d2dfa795..75df16fe96 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -443,6 +443,7 @@ virNetServerClient *virNetServerClientNew(unsigned long long id, return NULL;

if (!(client->privateData = privNew(client, privOpaque))) { + virNetMessageFree(client->rx);

Maybe it's better to put this line into `virNetServerClientDispose`?

virObjectUnref(client); return NULL; } diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index 3ddf9f0428..a813e821a3 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -409,11 +409,15 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, if (virNetMessageDecodePayload(msg, dispatcher->arg_filter, arg) < 0) goto error;

- if (!(identity = virNetServerClientGetIdentity(client))) + if (!(identity = virNetServerClientGetIdentity(client))) { + xdr_free(dispatcher->arg_filter, arg); goto error; + }

- if (virIdentitySetCurrent(identity) < 0) + if (virIdentitySetCurrent(identity) < 0) { + xdr_free(dispatcher->arg_filter, arg); goto error; + }

/* * When the RPC handler is called: @@ -427,8 +431,10 @@ virNetServerProgramDispatchCall(virNetServerProgram *prog, */ rv = (dispatcher->func)(server, client, msg, &rerr, arg, ret);

- if (virIdentitySetCurrent(NULL) < 0) + if (virIdentitySetCurrent(NULL) < 0) { + xdr_free(dispatcher->arg_filter, arg); goto error; + }

/* * If rv == 1, this indicates the dispatch func has

Thanks, Peng

On 09/09/2022 14:10, Jiacheng Jiang wrote:

From: jiangjiacheng <jiangjiacheng@huawei.com>

Function virTypedParamsAddString may return -1

I don't think `virTypedParamsAddString` will return -1 in the code paths you touched. The only case where `virTypedParamsAddString` returns -1 is that the `name` you pass to it is too long (>=80), however, in the code path you touched `name`s are hard-coded and the lengths of them are <80.

and the clean path doesn't free the memory of eventParams, which will lead to potential memory leak.

Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_driver.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 707f4cc1bb..c43bc4070e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4474,11 +4474,15 @@ qemuDomainPinVcpuLive(virDomainObj *vm, goto cleanup;

event = virDomainEventTunableNewFromObj(vm, eventParams, eventNparams); + eventParams = NULL; + eventNparams = 0;

ret = 0;

cleanup: virObjectEventStateQueue(driver->domainEventState, event); + if (eventParams) + virTypedParamsFree(eventParams, eventNparams); return ret; }

@@ -4683,6 +4687,8 @@ qemuDomainPinEmulator(virDomainPtr dom, goto endjob;

event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); + eventParams = NULL; + eventNparams = 0; }

if (persistentDef) { @@ -4699,6 +4705,8 @@ qemuDomainPinEmulator(virDomainPtr dom, qemuDomainObjEndJob(vm);

cleanup: + if (eventParams) + virTypedParamsFree(eventParams, eventNparams); virObjectEventStateQueue(driver->domainEventState, event); virDomainObjEndAPI(&vm); return ret; @@ -5080,6 +5088,8 @@ qemuDomainPinIOThread(virDomainPtr dom, goto endjob;

event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); + eventParams = NULL; + eventNparams = 0; }

if (persistentDef) { @@ -5105,6 +5115,8 @@ qemuDomainPinIOThread(virDomainPtr dom, qemuDomainObjEndJob(vm);

cleanup: + if (eventParams) + virTypedParamsFree(eventParams, eventNparams); virObjectEventStateQueue(driver->domainEventState, event); virDomainObjEndAPI(&vm); return ret;

On 09/09/2022 14:10, Jiacheng Jiang wrote:

From: jiangjiacheng <jiangjiacheng@huawei.com>

The qemuDomainAttachDeviceLive interface is invoked for attaching cdrom in the same way as common disks. The difference is that attach cdrom only update the src of the original device while common disk will add new disk to vm's device list. Therefore, the dev->data.disk should be freed to avoid memory leak when attach cdrom as well as floppy.

I think your colleague has fixed it in 2f470a4fb1e ("qemu: fix memleak in qemuDomainAttachDeviceLive()"). But I think there might be another UAF problem in the code. If updating the src of cdrom/floppy successfully, then dev->data.disk should be freed in `qemuDomainAttachDeviceDiskLive`, however we access dev->data.disk->info.alias after that. Thanks, Peng

Signed-off-by: jiangjiacheng <jiangjiacheng@huawei.com> --- src/qemu/qemu_driver.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c43bc4070e..64b1ca3f39 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6882,6 +6882,9 @@ qemuDomainAttachDeviceLive(virDomainObj *vm, ret = qemuDomainAttachDeviceDiskLive(driver, vm, dev); if (!ret) { alias = dev->data.disk->info.alias; + if ((virDomainDiskDevice)dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM || + (virDomainDiskDevice)dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) + virDomainDiskDefFree(dev->data.disk); dev->data.disk = NULL; } break;