[PATCH] virDevMapperGetTargetsImpl: quit early if device is not a devmapper target

Michal Privoznik posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ca7b09f054131be2aaaa147940b712add9abd723.1589195710.git.mprivozn@redhat.com
src/util/virdevmapper.c | 10 ++++++++++
1 file changed, 10 insertions(+)
[PATCH] virDevMapperGetTargetsImpl: quit early if device is not a devmapper target
Posted by Michal Privoznik 3 years, 10 months ago
As suggested in the linked bug, libvirt should firstly check
whether the major number of the device is device mapper major.
Because if it isn't subsequent DM_DEVICE_DEPS task may not only
fail, but also yield different results. In the bugzilla this is
demonstrated by creating a devmapper target named 'loop0' and
then creating loop target /dev/loop0. When the latter is then
passed to a domain, our virDevMapperGetTargetsImpl() function
blindly asks devmapper to provide target dependencies for
/dev/loop0 and because of the way devmapper APIs work, it will
'sanitize' the input by using the last component only which is
'loop0' and thus return different results than expected.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1823976

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/util/virdevmapper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
index feb5982315..ee026d502e 100644
--- a/src/util/virdevmapper.c
+++ b/src/util/virdevmapper.c
@@ -64,6 +64,7 @@ virDevMapperGetTargetsImpl(const char *path,
                            char ***devPaths_ret,
                            unsigned int ttl)
 {
+    struct stat sb;
     struct dm_task *dmt = NULL;
     struct dm_deps *deps;
     struct dm_info info;
@@ -82,6 +83,15 @@ virDevMapperGetTargetsImpl(const char *path,
         return ret;
     }
 
+    if (stat(path, &sb) < 0) {
+        if (errno == ENOENT)
+            ret = 0;
+        return ret;
+    }
+
+    if (!dm_is_dm_major(major(sb.st_dev)))
+        return 0;
+
     if (!(dmt = dm_task_create(DM_DEVICE_DEPS))) {
         if (errno == ENOENT || errno == ENODEV) {
             /* It's okay. Kernel is probably built without
-- 
2.26.2

Re: [PATCH] virDevMapperGetTargetsImpl: quit early if device is not a devmapper target
Posted by Ján Tomko 3 years, 10 months ago
On a Monday in 2020, Michal Privoznik wrote:
>As suggested in the linked bug, libvirt should firstly check
>whether the major number of the device is device mapper major.
>Because if it isn't subsequent DM_DEVICE_DEPS task may not only
>fail, but also yield different results. In the bugzilla this is
>demonstrated by creating a devmapper target named 'loop0' and
>then creating loop target /dev/loop0. When the latter is then
>passed to a domain, our virDevMapperGetTargetsImpl() function
>blindly asks devmapper to provide target dependencies for
>/dev/loop0 and because of the way devmapper APIs work, it will
>'sanitize' the input by using the last component only which is
>'loop0' and thus return different results than expected.
>
>Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1823976
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/util/virdevmapper.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c
>index feb5982315..ee026d502e 100644
>--- a/src/util/virdevmapper.c
>+++ b/src/util/virdevmapper.c
>@@ -64,6 +64,7 @@ virDevMapperGetTargetsImpl(const char *path,
>                            char ***devPaths_ret,
>                            unsigned int ttl)
> {
>+    struct stat sb;
>     struct dm_task *dmt = NULL;
>     struct dm_deps *deps;
>     struct dm_info info;
>@@ -82,6 +83,15 @@ virDevMapperGetTargetsImpl(const char *path,
>         return ret;
>     }
>
>+    if (stat(path, &sb) < 0) {
>+        if (errno == ENOENT)
>+            ret = 0;
>+        return ret;

Why not 'return 0' and 'return -1' directly?

>+    }
>+
>+    if (!dm_is_dm_major(major(sb.st_dev)))
>+        return 0;
>+

Either way
Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano