There are no checks, virDomainChrSourceDefNew can return 0

[sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0

Posted by Миронов Сергей Владимирович 2 years, 4 months ago

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

Re: [sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0

Posted by Michal Prívozník 2 years, 4 months ago

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

Re: [sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0

Posted by Peter Krempa 2 years, 4 months ago

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

Re: [sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0

Posted by Миронов Сергей Владимирович 2 years, 4 months ago

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

Re: [sdl-qemu] [PATCH 0/1] There are no checks, virDomainChrSourceDefNew can return 0

Posted by Peter Krempa 2 years, 4 months ago

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.