[sdl-qemu] [PATCH 1/1] No checks, dereferencing possible

[sdl-qemu] [PATCH 1/1] No checks, dereferencing possible

[Миронов Сергей Владимирович]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

No checks, dereferencing possible.


Return value of a function 'virDomainChrSourceDefNew'
is dereferenced at qemu_command.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_command.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index e84374b4cf..8d11972c88 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -4698,6 +4698,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,
             g_autofree char *name = g_strdup_printf("%s-vhost-user", video->info.alias);
             qemuDomainChrSourcePrivate *chrsrcpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chrsrc);

+           if (chrsrc == NULL)
+                   return -1;
             chrsrc->type = VIR_DOMAIN_CHR_TYPE_UNIX;
             chrsrcpriv->directfd = qemuFDPassDirectNew(name, &videopriv->vhost_user_fd);
--
2.31.1

[-- Attachment #2: Type: text/html, Size: 8560 bytes --]

Re: [sdl-qemu] [PATCH 1/1] No checks, dereferencing possible

[Peter Krempa]

On Thu, Sep 14, 2023 at 09:44:16 +0000, Миронов Сергей Владимирович wrote:
> No checks, dereferencing possible.
> 
> 
> Return value of a function 'virDomainChrSourceDefNew'
> is dereferenced at qemu_command.c without checking
> for NULL, but it is usually checked for this function.

This description here doesn't make sense. You are checking the presence
of 'privateData' in 'virDomainVideoDef'.

> 
> 
> 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_command.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index e84374b4cf..8d11972c88 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4698,6 +4698,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,
>              g_autofree char *name = g_strdup_printf("%s-vhost-user", video->info.alias);
>              qemuDomainChrSourcePrivate *chrsrcpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chrsrc);
> 
> +           if (chrsrc == NULL)
> +                   return -1;

This addition doesn't make sense as it's dead code. The private data is
always allocated and checked that it's non-NULL in the qemu driver via
the callback in virDomainVideoDefNew.

Do you have a call trace that would prove me otherwise?

Re: [sdl-qemu] [PATCH 1/1] No checks, dereferencing possible

[Daniel P. Berrangé]

On Thu, Sep 14, 2023 at 12:22:26PM +0200, Peter Krempa wrote:
> On Thu, Sep 14, 2023 at 09:44:16 +0000, Миронов Сергей Владимирович wrote:
> > No checks, dereferencing possible.
> > 
> > 
> > Return value of a function 'virDomainChrSourceDefNew'
> > is dereferenced at qemu_command.c without checking
> > for NULL, but it is usually checked for this function.
> 
> This description here doesn't make sense. You are checking the presence
> of 'privateData' in 'virDomainVideoDef'.
> 
> > 
> > 
> > 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_command.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index e84374b4cf..8d11972c88 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4698,6 +4698,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,
> >              g_autofree char *name = g_strdup_printf("%s-vhost-user", video->info.alias);
> >              qemuDomainChrSourcePrivate *chrsrcpriv = QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chrsrc);
> > 
> > +           if (chrsrc == NULL)
> > +                   return -1;
> 
> This addition doesn't make sense as it's dead code. The private data is
> always allocated and checked that it's non-NULL in the qemu driver via
> the callback in virDomainVideoDefNew.


This is checking the result of  virDomainChrSourceDefNew(), which can
fail as virDomainChrSourceDefInitialize() can fail if virClassNew()
fails its sanity checks.

The check is too late though, as we de-referenced chrsrc in the line
above when we accessed the private data.

> 
> Do you have a call trace that would prove me otherwise?
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|