Re: [Qemu-devel] [PATCH v6 4/8] module: implement module loading function

[Daniel P. Berrange]

On Wed, Sep 11, 2013 at 09:34:04PM +0800, Fam Zheng wrote:
> Added three types of modules:
> 
>     typedef enum {
>         MODULE_LOAD_BLOCK = 0,
>         MODULE_LOAD_UI,
>         MODULE_LOAD_NET,
>         MODULE_LOAD_MAX,
>     } module_load_type;
> 
> and their loading function:
> 
>     void module_load(module_load_type).
> 
> which loads all ".so" files in a subdir under "${PREFIX}/qemu/", e.g.
> "/usr/lib/qemu/block". Modules of each type should be loaded before
> respective subsystem initialization code.
> 
> Requires gmodule-2.0 from glib.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block.c               |  1 +
>  bsd-user/main.c       |  3 +++
>  configure             | 28 ++++++++++++++++++---------
>  include/qemu/module.h |  9 +++++++++
>  linux-user/main.c     |  3 +++
>  scripts/create_config |  7 +++++++
>  util/module.c         | 53 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  vl.c                  |  2 ++
>  8 files changed, 97 insertions(+), 9 deletions(-)

After this change is applied, if you don't pass --enable-modules to
confoigure, then QEMU spams stdout at startup

  Failed to open dir /home/berrange/usr/qemu-git/lib/qemu/ui/
  Failed to open dir /home/berrange/usr/qemu-git/lib/qemu/net/
  Failed to open dir /home/berrange/usr/qemu-git/lib/qemu/block/


If I do enable modules, QEMU still complains about the ui/ & net/
directories not existing.



> +    dp = opendir(path);
> +    if (!dp) {
> +        fprintf(stderr, "Failed to open dir %s\n", path);
> +        return;
> +    }
> +    for (ep = readdir(dp); ep; ep = readdir(dp)) {

By dynamically loading all modules found in the directory, with
not validity checks this opens the doorway for 3rd party vendors
to drop-in closed source modules to QEMU binaries.

Anthony's spec (http://wiki.qemu.org/Features/Modules) had said

 "What this is not

    A mechanism to support third party extensions to QEMU or
    out of tree drivers/features
    A stable interface
    A GPL barrier 

  This system should not be (ab)used to allow 3rd-party modules
  to be loaded into qemu, especially to "work around" GPL restrictions.
  In order to ensure this, the modules system should be built in a way
  to allow loading only modules which were built together with qemu,
  by adding, for example, hashes of current build to the main exported
  symbols."


We know the precise list of valid modules when building QEMU,
so IMHO, this should just explicitly load each known module
name, and *not* readdir. Also it should do something along the
lines suggested their of poisoning exported symbols with a
build hash to guarantee the modules loaded match the original
binary and that the symbols change on every rebuild.

The latter is important even ignoring the 3rd party module
question, since it ensures developers/users don't accidently
run with mis-match QEMU and module builds, which could lead
to some very hard to diagnose bugs / behaviour.

> +        int len = strlen(ep->d_name);
> +        if (len > suf_len &&
> +                !strcmp(&ep->d_name[len - suf_len], dsosuf)) {
> +            fname = g_strdup_printf("%s%s", path, ep->d_name);
> +            g_module = g_module_open(fname,
> +                                     G_MODULE_BIND_LAZY | 
> G_MODULE_BIND_LOCAL);
> +            if (!g_module) {
> +                fprintf(stderr, "Failed to open module file %s\n",
> +                        g_module_error());
> +                g_free(fname);
> +                continue;
> +            }
> +            g_free(fname);
> +        }
> +    }
> +}


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|