[sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0
There are no checks, virDomainChrSourceDefNew can return 0. Return value of a function 'virDomainChrSourceDefNew' is dereferenced at qemu_hotplug.c without checking for NULL, but it is usually checked for this function. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 1f85f0967b ("ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'") Signed-off-by: Sergey Mironov <mironov@fintech.ru> --- src/qemu/qemu_hotplug.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 177ca87d11..09e16c2c7e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3207,6 +3207,8 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver, qemuAssignDeviceFSAlias(vm->def, fs); chardev = virDomainChrSourceDefNew(priv->driver->xmlopt); + if (chardev == NULL) + goto cleanup; chardev->type = VIR_DOMAIN_CHR_TYPE_UNIX; chardev->data.nix.path = qemuDomainGetVHostUserFSSocketPath(priv, fs); -- 2.31.1
CC-ing qemu-devel with a patch solely for libvirt doesn't make sense. Also 'libvirt-security' list is private and is is intended as a first contact list for stuff to be embargoed. It makes little sense to include it when posting to the public 'libvir-list'. On Thu, Sep 14, 2023 at 09:44:13 +0000, Миронов Сергей Владимирович wrote:
There are no checks, virDomainChrSourceDefNew can return 0.
s/0/NULL While very technically true, realistically that can't happen any more. 'virObjectNew' always returns a valid pointer or abort()s, and VIR_CLASS_NEW can return 0 on programming errors. Thus this is not a security issue.
Return value of a function 'virDomainChrSourceDefNew'
is dereferenced at qemu_hotplug.c without checking for NULL,
but it is usually checked for this function.
Remove the extra empty lines please.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 1f85f0967b ("ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'")
^^ This makes no sense. The commit you are referencing is changing a shell script.
Signed-off-by: Sergey Mironov <mironov@fintech.ru>
--- src/qemu/qemu_hotplug.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 177ca87d11..09e16c2c7e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3207,6 +3207,8 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver, qemuAssignDeviceFSAlias(vm->def, fs);
chardev = virDomainChrSourceDefNew(priv->driver->xmlopt); + if (chardev == NULL) + goto cleanup; chardev->type = VIR_DOMAIN_CHR_TYPE_UNIX; chardev->data.nix.path = qemuDomainGetVHostUserFSSocketPath(priv, fs); -- 2.31.1
Okay, Peter Should I resend the patches correctly or wait for a response for those added to the mailing list? https://listman.redhat.com/archives/libvir-list/2023-September/242078.html https://listman.redhat.com/archives/libvir-list/2023-September/242077.html ________________________________ От: Peter Krempa <pkrempa@redhat.com> Отправлено: 14 сентября 2023 г. 13:17 Кому: Миронов Сергей Владимирович Копия: libvirt-security@redhat.com; qemu-devel@nongnu.org; libvir-list@redhat.com; sdl.qemu@linuxtesting.org Тема: Re: [sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0 CC-ing qemu-devel with a patch solely for libvirt doesn't make sense. Also 'libvirt-security' list is private and is is intended as a first contact list for stuff to be embargoed. It makes little sense to include it when posting to the public 'libvir-list'. On Thu, Sep 14, 2023 at 09:44:13 +0000, Миронов Сергей Владимирович wrote:
There are no checks, virDomainChrSourceDefNew can return 0.
s/0/NULL While very technically true, realistically that can't happen any more. 'virObjectNew' always returns a valid pointer or abort()s, and VIR_CLASS_NEW can return 0 on programming errors. Thus this is not a security issue.
Return value of a function 'virDomainChrSourceDefNew'
is dereferenced at qemu_hotplug.c without checking for NULL,
but it is usually checked for this function.
Remove the extra empty lines please.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 1f85f0967b ("ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'")
^^ This makes no sense. The commit you are referencing is changing a shell script.
Signed-off-by: Sergey Mironov <mironov@fintech.ru>
--- src/qemu/qemu_hotplug.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 177ca87d11..09e16c2c7e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3207,6 +3207,8 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver, qemuAssignDeviceFSAlias(vm->def, fs);
chardev = virDomainChrSourceDefNew(priv->driver->xmlopt); + if (chardev == NULL) + goto cleanup; chardev->type = VIR_DOMAIN_CHR_TYPE_UNIX; chardev->data.nix.path = qemuDomainGetVHostUserFSSocketPath(priv, fs); -- 2.31.1
On Thu, Sep 14, 2023 at 13:45:23 +0000, Миронов Сергей Владимирович wrote: This is a technical list so please avoid top-posting.
Okay, Peter
Should I resend the patches correctly or wait for a response for those added to the mailing list?
https://listman.redhat.com/archives/libvir-list/2023-September/242078.html
This patch make some sense. Please resend it with my and Michal's comments addressed.
https://listman.redhat.com/archives/libvir-list/2023-September/242077.html
This patch IMO doesn't make sense at all. Unless you have a proper justification for it, e.g. a call trace which would prove my assumption wrong, then there's no need to post that. If you do have a justification for it, please put it into the commit message and repost that one as well. Note that my comments from 0/1 apply also to 1/1.
On 9/14/23 11:44, Миронов Сергей Владимирович wrote:
There are no checks, virDomainChrSourceDefNew can return 0.
Return value of a function 'virDomainChrSourceDefNew'
is dereferenced at qemu_hotplug.c without checking for NULL,
but it is usually checked for this function.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 1f85f0967b ("ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'")
Signed-off-by: Sergey Mironov <mironov@fintech.ru>
--- src/qemu/qemu_hotplug.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 177ca87d11..09e16c2c7e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3207,6 +3207,8 @@ qemuDomainAttachFSDevice(virQEMUDriver *driver, qemuAssignDeviceFSAlias(vm->def, fs);
chardev = virDomainChrSourceDefNew(priv->driver->xmlopt); + if (chardev == NULL) + goto cleanup; chardev->type = VIR_DOMAIN_CHR_TYPE_UNIX; chardev->data.nix.path = qemuDomainGetVHostUserFSSocketPath(priv, fs);
Apart from Peter's findings, there are more cases like this: qemuBuildVideoCommandLine(), qemuVirtioFSOpenChardev(), qemuMonitorJSONTestAttachChardev(). Might be worth fixing them too. Michal
participants (3)
-
Michal Prívozník -
Peter Krempa -
Миронов Сергей Владимирович