[PATCH] qemu-config: load modules when instantiating option groups

[PATCH] qemu-config: load modules when instantiating option groups

[Paolo Bonzini]

Right now the SPICE module is special cased to be loaded when processing
of the -spice command line option.  However, the spice option group
can also be brought in via -readconfig, in which case the module is
not loaded.

Add a generic hook to load modules that provide a QemuOpts group,
and use it for the "spice" and "iscsi" groups.

Fixes: #194
Fixes: https://bugs.launchpad.net/qemu/+bug/1910696
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/qemu/config-file.h |  2 +-
 softmmu/vl.c               | 21 +++++++++++++++++----
 stubs/meson.build          |  1 +
 stubs/module-opts.c        |  6 ++++++
 util/qemu-config.c         |  1 +
 5 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100644 stubs/module-opts.c

diff --git a/include/qemu/config-file.h b/include/qemu/config-file.h
index 8d3e53ae4d..0500b3668d 100644
--- a/include/qemu/config-file.h
+++ b/include/qemu/config-file.h
@@ -1,7 +1,7 @@
 #ifndef QEMU_CONFIG_FILE_H
 #define QEMU_CONFIG_FILE_H
 
-
+void qemu_load_module_for_opts(const char *group);
 QemuOptsList *qemu_find_opts(const char *group);
 QemuOptsList *qemu_find_opts_err(const char *group, Error **errp);
 QemuOpts *qemu_find_opts_singleton(const char *group);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 11ac3750d8..d55424e634 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2615,6 +2615,23 @@ void qmp_x_exit_preconfig(Error **errp)
     }
 }
 
+#ifdef CONFIG_MODULES
+void qemu_load_module_for_opts(const char *group)
+{
+    static bool spice_tried = false;
+    if (g_str_equal(group, "spice") && !spice_tried) {
+        ui_module_load_one("spice-core");
+        spice_tried = true;
+    }
+
+    static bool iscsi_tried = false;
+    if (g_str_equal(group, "iscsi") && !iscsi_tried) {
+        block_module_load_one("iscsi");
+        iscsi_tried = true;
+    }
+}
+#endif
+
 void qemu_init(int argc, char **argv, char **envp)
 {
     QemuOpts *opts;
@@ -3374,10 +3391,6 @@ void qemu_init(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_spice:
                 olist = qemu_find_opts_err("spice", NULL);
-                if (!olist) {
-                    ui_module_load_one("spice-core");
-                    olist = qemu_find_opts("spice");
-                }
                 if (!olist) {
                     error_report("spice support is disabled");
                     exit(1);
diff --git a/stubs/meson.build b/stubs/meson.build
index 3faef16892..f3f979c3fe 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -22,6 +22,7 @@ stub_ss.add(files('isa-bus.c'))
 stub_ss.add(files('is-daemonized.c'))
 stub_ss.add(when: 'CONFIG_LINUX_AIO', if_true: files('linux-aio.c'))
 stub_ss.add(files('migr-blocker.c'))
+stub_ss.add(files('module-opts.c'))
 stub_ss.add(files('monitor.c'))
 stub_ss.add(files('monitor-core.c'))
 stub_ss.add(files('pci-bus.c'))
diff --git a/stubs/module-opts.c b/stubs/module-opts.c
new file mode 100644
index 0000000000..a7d0e4ad6e
--- /dev/null
+++ b/stubs/module-opts.c
@@ -0,0 +1,6 @@
+#include "qemu/osdep.h"
+#include "qemu/config-file.h"
+
+void qemu_load_module_for_opts(const char *group)
+{
+}
diff --git a/util/qemu-config.c b/util/qemu-config.c
index 670bd6ebca..34974c4b47 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -16,6 +16,7 @@ static QemuOptsList *find_list(QemuOptsList **lists, const char *group,
 {
     int i;
 
+    qemu_load_module_for_opts(group);
     for (i = 0; lists[i] != NULL; i++) {
         if (strcmp(lists[i]->name, group) == 0)
             break;
-- 
2.27.0

Re: [PATCH] qemu-config: load modules when instantiating option groups

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210518131542.2941207-1-pbonzini@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210518131542.2941207-1-pbonzini@redhat.com
Subject: [PATCH] qemu-config: load modules when instantiating option groups

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   8e22b27..15e147b  master     -> master
 - [tag update]      patchew/20210517064428.16223-1-vsementsov@virtuozzo.com -> patchew/20210517064428.16223-1-vsementsov@virtuozzo.com
 * [new tag]         patchew/20210518131542.2941207-1-pbonzini@redhat.com -> patchew/20210518131542.2941207-1-pbonzini@redhat.com
Switched to a new branch 'test'
caeb56b qemu-config: load modules when instantiating option groups

=== OUTPUT BEGIN ===
ERROR: do not initialise statics to 0 or NULL
#51: FILE: softmmu/vl.c:2620:
+    static bool spice_tried = false;

ERROR: do not initialise statics to 0 or NULL
#57: FILE: softmmu/vl.c:2626:
+    static bool iscsi_tried = false;

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#92: 
new file mode 100644

total: 2 errors, 1 warnings, 61 lines checked

Commit caeb56b0e05f (qemu-config: load modules when instantiating option groups) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210518131542.2941207-1-pbonzini@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH] qemu-config: load modules when instantiating option groups

[Gerd Hoffmann]

On Tue, May 18, 2021 at 09:15:42AM -0400, Paolo Bonzini wrote:
> Right now the SPICE module is special cased to be loaded when processing
> of the -spice command line option.  However, the spice option group
> can also be brought in via -readconfig, in which case the module is
> not loaded.
> 
> Add a generic hook to load modules that provide a QemuOpts group,
> and use it for the "spice" and "iscsi" groups.
> 
> Fixes: #194
> Fixes: https://bugs.launchpad.net/qemu/+bug/1910696
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

Automatic module loading (was: [PATCH] qemu-config: load modules when instantiating option groups)

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Right now the SPICE module is special cased to be loaded when processing
> of the -spice command line option.  However, the spice option group
> can also be brought in via -readconfig, in which case the module is
> not loaded.
>
> Add a generic hook to load modules that provide a QemuOpts group,
> and use it for the "spice" and "iscsi" groups.
>
> Fixes: #194
> Fixes: https://bugs.launchpad.net/qemu/+bug/1910696
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

What follows is not an objection to this patch.

I think we have this kind of bugs because we're kind of wobbly on when
to load modules.

On the one hand, we're trying to load modules only when needed.  This is
obviously useful to conserve resources, and to keep the attack surface
small.  Some background in

    Message-ID: <20210409064642.ah2tz5vjz2ngfiyo@sirius.home.kraxel.org>
    https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01393.html

On the other hand, we're trying to make modules transparent to
management applications, i.e. QEMU looks the same whether something was
compiled as a loadable module or linked into QEMU itself.  See

    Message-ID: <YHAhQWdX15V54U8G@redhat.com>
    https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01450.html

I'm afraid we sort of fail at both.

Transparency to management applications requires us to load modules on
QOM introspection already.

Example: to answer "show me all QOM types", we need to load all modules
that could possibly register QOM types.  As long as module code can do
whatever it wants, that means loading all of them.

Example: to answer "show me QOM type FOO", where FOO is currently
unknown, we need to load all modules that could possible register QOM
type FOO.  Again, that means loading all of them.

We don't actually do this.  Instead, we hardcode a map from type name to
module name[*], so we don't have to load them all, and we actually load
the module specified by this map only sometimes, namely when we call
module_object_class_by_name() instead of object_class_by_name().  I
can't discern rules when to call which one.  Wobbly.

Things other than QOM might be affected, too.

QAPI introspection is not: the value of query-qmp-schema is fixed at
compile-time, and *how* something is compiled (loadable module
vs. linked into QEMU itself) does not affect it.

I'd like us to develop a clearer understanding when exactly modules are
to be loaded.


[*] qom_modules[] in util/module.c.  This is a basically an (unchecked)
assertion that the (unrelated!) module code won't register anything
else.  Ugh!

Re: Automatic module loading (was: [PATCH] qemu-config: load modules when instantiating option groups)

[Daniel P. Berrangé]

On Wed, May 19, 2021 at 03:14:54PM +0200, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > Right now the SPICE module is special cased to be loaded when processing
> > of the -spice command line option.  However, the spice option group
> > can also be brought in via -readconfig, in which case the module is
> > not loaded.
> >
> > Add a generic hook to load modules that provide a QemuOpts group,
> > and use it for the "spice" and "iscsi" groups.
> >
> > Fixes: #194
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1910696
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> What follows is not an objection to this patch.
> 
> I think we have this kind of bugs because we're kind of wobbly on when
> to load modules.
> 
> On the one hand, we're trying to load modules only when needed.  This is
> obviously useful to conserve resources, and to keep the attack surface
> small.  Some background in
> 
>     Message-ID: <20210409064642.ah2tz5vjz2ngfiyo@sirius.home.kraxel.org>
>     https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01393.html

I'm not convinced by the runtime attack surface argument, because even
if QEMU doesn't auto-load, if the attacker has gained enough control
over QEMU that they can jump into code that is not otherwise enabled
by QEMU config, then they can likely scribble into RAM in a way that
triggers dlopen() of any .so on the filesystem, whether a QEMU module
or not.

IMHO the main benefit of modules is that you can avoid installing
everything on disk, and thus avoid pulling in a long chain of deps,
and thus get smaller containers, and avoid having to worry about
software updates for CVEs in things you're not using.

> On the other hand, we're trying to make modules transparent to
> management applications, i.e. QEMU looks the same whether something was
> compiled as a loadable module or linked into QEMU itself.  See
> 
>     Message-ID: <YHAhQWdX15V54U8G@redhat.com>
>     https://lists.gnu.org/archive/html/qemu-devel/2021-04/msg01450.html
> 
> I'm afraid we sort of fail at both.
>
> Transparency to management applications requires us to load modules on
> QOM introspection already.

Yes, and bugs in the latter have already caused mis-behavior in
libvirt.


> Example: to answer "show me all QOM types", we need to load all modules
> that could possibly register QOM types.  As long as module code can do
> whatever it wants, that means loading all of them.
> 
> Example: to answer "show me QOM type FOO", where FOO is currently
> unknown, we need to load all modules that could possible register QOM
> type FOO.  Again, that means loading all of them.
> 
> We don't actually do this.  Instead, we hardcode a map from type name to
> module name[*], so we don't have to load them all, and we actually load
> the module specified by this map only sometimes, namely when we call
> module_object_class_by_name() instead of object_class_by_name().  I
> can't discern rules when to call which one.  Wobbly.
> 
> Things other than QOM might be affected, too.
> 
> QAPI introspection is not: the value of query-qmp-schema is fixed at
> compile-time, and *how* something is compiled (loadable module
> vs. linked into QEMU itself) does not affect it.
> 
> I'd like us to develop a clearer understanding when exactly modules are
> to be loaded.

Agreed, we need much better defined behaviour here.

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 :|