[PATCH] lxc: fix lxcContainerMountAllFS() DEREF_BEFORE_CHECK
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
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
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
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
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
participants (4)
-
Dmitry Frolov -
Jiri Denemark -
Martin Kletzander -
Peter Krempa