1. Patchew
  2. Libvirt
  3. View series

[PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK

Dmitry Frolov posted 1 patch 7 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20230906153443.207740-1-frolov@swemel.ru
There is a newer version of this series
src/lxc/lxc_container.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK
Posted by Dmitry Frolov 7 months, 3 weeks ago 
Reviewing the sources, I found, that
in function lxcContainerMountAllFS() pointers
vmDef->fss[i]->src and vmDef->fss[i]->src->path
are checked for NULL after dereferencing in
VIR_DEBUG() macro.

Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting filesystems")
---
 src/lxc/lxc_container.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
index 21220661f7..58a6695458 100644
--- a/src/lxc/lxc_container.c
+++ b/src/lxc/lxc_container.c
@@ -1467,13 +1467,15 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
         if (STREQ(vmDef->fss[i]->dst, "/"))
             continue;
 
+        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path))
+            return -1;
+
         VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src->path, vmDef->fss[i]->dst);
 
         if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
             return -1;
 
-        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
-              STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
+        if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst) &&
             lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
             return -1;
 
-- 
2.34.1
Re: [PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK
Posted by Jiri Denemark 7 months, 3 weeks ago 
On Wed, Sep 06, 2023 at 18:34:42 +0300, Dmitry Frolov wrote:
> Reviewing the sources, I found, that
> in function lxcContainerMountAllFS() pointers
> vmDef->fss[i]->src and vmDef->fss[i]->src->path
> are checked for NULL after dereferencing in
> VIR_DEBUG() macro.
> 
> Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting filesystems")
> ---
>  src/lxc/lxc_container.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 21220661f7..58a6695458 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -1467,13 +1467,15 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
>          if (STREQ(vmDef->fss[i]->dst, "/"))
>              continue;
>  
> +        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path))
> +            return -1;

This would return -1 without reporting any error. And you are also
changing the logic here without explaining it. If vmDef->fss[i]->src was
NULL or vmDef->fss[i]->src->path was NULL this function would call
lxcContainerUnmountSubtree on vmDef->fss[i]->dst before, but now it just
returns -1. Well except that it would crash first :-) In any case, this
part is wrong, see below...

> +
>          VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src->path, vmDef->fss[i]->dst);
>  
>          if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
>              return -1;
>  
> -        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
> -              STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
> +        if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst) &&
>              lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
>              return -1;

I checked our parsing code and vmDef->fss[i]->src can never be NULL as
it is always allocated in virDomainFSDefNew so checking it is redundant.
On the other hand vmDef->fss[i]->src->path can be NULL, but it is not en
error, e.g., VIR_DOMAIN_FS_TYPE_RAM would contain path == NULL.

So the fix should rather remove the check for vmDef->fss[i]->src and use
NULLSTR(vmDef->fss[i]->src->path) in VIR_DEBUG.

Jirka
Re: [PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK
Posted by Martin Kletzander 7 months, 3 weeks ago 
On Wed, Sep 06, 2023 at 06:34:42PM +0300, Dmitry Frolov wrote:
>Reviewing the sources, I found, that
>in function lxcContainerMountAllFS() pointers
>vmDef->fss[i]->src and vmDef->fss[i]->src->path
>are checked for NULL after dereferencing in
>VIR_DEBUG() macro.
>
>Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting filesystems")
>---
> src/lxc/lxc_container.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
>diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
>index 21220661f7..58a6695458 100644
>--- a/src/lxc/lxc_container.c
>+++ b/src/lxc/lxc_container.c
>@@ -1467,13 +1467,15 @@ static int lxcContainerMountAllFS(virDomainDef *vmDef,
>         if (STREQ(vmDef->fss[i]->dst, "/"))
>             continue;
>
>+        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path))
>+            return -1;
>+

This unfortunately changes the semantics.  This fails whenever there is
src or src->path missing, but ...

>         VIR_DEBUG("Mounting '%s' -> '%s'", vmDef->fss[i]->src->path, vmDef->fss[i]->dst);
>
>         if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
>             return -1;
>
>-        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
>-              STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
>+        if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst) &&
>             lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)

... here it only skips the unmount in such case.  That makes sense
because for example tmpfs does not have a source path, only the
destination and the mount that is not visible in the context here would
not only be skipped as well, but the whole starting of a domain would
fail.

Instead of this patch, you could just conditionally change the
VIR_DEBUG() to report what's actually happening.

>             return -1;
>
>-- 
>2.34.1
>
Re: [PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK
Posted by Peter Krempa 7 months, 3 weeks ago 
On Thu, Sep 07, 2023 at 09:11:34 +0200, Martin Kletzander wrote:
> On Wed, Sep 06, 2023 at 06:34:42PM +0300, Dmitry Frolov wrote:
> > Reviewing the sources, I found, that
> > in function lxcContainerMountAllFS() pointers
> > vmDef->fss[i]->src and vmDef->fss[i]->src->path
> > are checked for NULL after dereferencing in
> > VIR_DEBUG() macro.
> > 
> > Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting filesystems")
> > ---
> > src/lxc/lxc_container.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)

[...]

> >         if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
> >             return -1;
> > 
> > -        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
> > -              STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
> > +        if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst) &&
> >             lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
> 
> ... here it only skips the unmount in such case.  That makes sense
> because for example tmpfs does not have a source path, only the
> destination and the mount that is not visible in the context here would
> not only be skipped as well, but the whole starting of a domain would
> fail.
> 
> Instead of this patch, you could just conditionally change the
> VIR_DEBUG() to report what's actually happening.

And we have a handy NULLSTR() macro for that case.

> 
> >             return -1;
> > 
> > -- 
> > 2.34.1
> >
Re: [PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK
Posted by Martin Kletzander 7 months, 3 weeks ago 
On Thu, Sep 07, 2023 at 09:25:14AM +0200, Peter Krempa wrote:
>On Thu, Sep 07, 2023 at 09:11:34 +0200, Martin Kletzander wrote:
>> On Wed, Sep 06, 2023 at 06:34:42PM +0300, Dmitry Frolov wrote:
>> > Reviewing the sources, I found, that
>> > in function lxcContainerMountAllFS() pointers
>> > vmDef->fss[i]->src and vmDef->fss[i]->src->path
>> > are checked for NULL after dereferencing in
>> > VIR_DEBUG() macro.
>> >
>> > Fixes: 57487085dc ("lxc: don't try to reference NULL when mounting filesystems")
>> > ---
>> > src/lxc/lxc_container.c | 6 ++++--
>> > 1 file changed, 4 insertions(+), 2 deletions(-)
>
>[...]
>
>> >         if (lxcContainerResolveSymlinks(vmDef->fss[i], false) < 0)
>> >             return -1;
>> >
>> > -        if (!(vmDef->fss[i]->src && vmDef->fss[i]->src->path &&
>> > -              STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst)) &&
>> > +        if (!STRPREFIX(vmDef->fss[i]->src->path, vmDef->fss[i]->dst) &&
>> >             lxcContainerUnmountSubtree(vmDef->fss[i]->dst, false) < 0)
>>
>> ... here it only skips the unmount in such case.  That makes sense
>> because for example tmpfs does not have a source path, only the
>> destination and the mount that is not visible in the context here would
>> not only be skipped as well, but the whole starting of a domain would
>> fail.
>>
>> Instead of this patch, you could just conditionally change the
>> VIR_DEBUG() to report what's actually happening.
>
>And we have a handy NULLSTR() macro for that case.
>

NULLSTR(a->b) won't help you if a == NULL, unfortunately :(


>>
>> >             return -1;
>> >
>> > --
>> > 2.34.1
>> >
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.