Message ID | 1378906448-15834-5-git-send-email-famz@redhat.com |
---|---|
State | New |
Headers | show |
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 <famz@redhat.com> > --- > 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
--On 11 September 2013 16:48:41 +0100 "Daniel P. Berrange" <berrange@redhat.com> wrote: > 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. +1 Though in the interest of not having to recompile everything for one minor change a .c file in qemu whilst developing, perhaps we could just hash exported symbols and include that hash, rather than ensuring the symbols change on (literally) every rebuild. Else every dev will build with modules turned off!
On 09/11/2013 08:48 AM, Daniel P. Berrange wrote: > 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. We need not mangle the symbols, which could be complicated to actually implement, and irritating to work around within gdb. We could instead just add the build-id as a variable within the module. Read and compare the build-id after loading the module; unload and reject on mismatch. r~
On Wed, 09/11 11:46, Richard Henderson wrote: > On 09/11/2013 08:48 AM, Daniel P. Berrange wrote: > > 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. > > We need not mangle the symbols, which could be complicated to > actually implement, and irritating to work around within gdb. > Agree with this, some id or hash check should be enough. > We could instead just add the build-id as a variable within > the module. Read and compare the build-id after loading the > module; unload and reject on mismatch. > > > r~
12.09.2013 07:02, Fam Zheng wrote. > On Wed, 09/11 11:46, Richard Henderson wrote: >> On 09/11/2013 08:48 AM, Daniel P. Berrange wrote: >>> 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. >> >> We need not mangle the symbols, which could be complicated to >> actually implement, and irritating to work around within gdb. >> > Agree with this, some id or hash check should be enough. A solution which I proposed at the very beginning -- to export a "hashed" init function from modules, and call it from the main executable. Like, instead of, say, qemu_module_init(), call qemu_module_init_0xdeadbeaf(), where 0xdeadbeaf is a hash of some build-dependent value. This should be enough to keep it going. Ofcourse, if a module lacks this function, it should not be loaded. Thanks, /mjt
On Thu, Sep 12, 2013 at 09:36:43AM +0400, Michael Tokarev wrote: > 12.09.2013 07:02, Fam Zheng wrote. > >On Wed, 09/11 11:46, Richard Henderson wrote: > >>On 09/11/2013 08:48 AM, Daniel P. Berrange wrote: > >>>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. > >> > >>We need not mangle the symbols, which could be complicated to > >>actually implement, and irritating to work around within gdb. > >> > >Agree with this, some id or hash check should be enough. > > A solution which I proposed at the very beginning -- to export > a "hashed" init function from modules, and call it from the > main executable. Like, instead of, say, qemu_module_init(), > call qemu_module_init_0xdeadbeaf(), where 0xdeadbeaf is a > hash of some build-dependent value. This should be enough > to keep it going. > > Ofcourse, if a module lacks this function, it should not be > loaded. Yep, that would be a reasonable way todo this. THe current patches use attribute(constructor) so QEMU doesn't actually call any explicit init function after dlopen()ing. That could easily be changed though. Daniel
On 09/11/2013 11:36 PM, Michael Tokarev wrote: > > A solution which I proposed at the very beginning -- to export > a "hashed" init function from modules, and call it from the > main executable. Like, instead of, say, qemu_module_init(), > call qemu_module_init_0xdeadbeaf(), where 0xdeadbeaf is a > hash of some build-dependent value. This should be enough > to keep it going. And of course, since we store sources in git, you already have such a hash value at your disposal: $CC -DBUILD_HASH=$(git rev-parse HEAD) ... coupled with glue(qemu_module_init_, BUILD_HASH) where the only trick is to figure out how to bake in a hash when building from a released tarball rather than git.
On Thu, Sep 12, 2013 at 05:59:30AM -0600, Eric Blake wrote: > On 09/11/2013 11:36 PM, Michael Tokarev wrote: > > > > A solution which I proposed at the very beginning -- to export > > a "hashed" init function from modules, and call it from the > > main executable. Like, instead of, say, qemu_module_init(), > > call qemu_module_init_0xdeadbeaf(), where 0xdeadbeaf is a > > hash of some build-dependent value. This should be enough > > to keep it going. > > And of course, since we store sources in git, you already have such a > hash value at your disposal: > $CC -DBUILD_HASH=$(git rev-parse HEAD) ... > coupled with > glue(qemu_module_init_, BUILD_HASH) > where the only trick is to figure out how to bake in a hash when > building from a released tarball rather than git. IMHO we want this to change any time you do './configure', so I would not tie this to git hash - we don't want all users of particular release tar.gz to have the same hash regardless of configure options. We want a situation where any time a distro builds an RPM or equiv, a new hash is used. So to me generating it in 'configure' seems like a reasonable place. Daniel
diff --git a/block.c b/block.c index 26639e8..16ceaaf 100644 --- a/block.c +++ b/block.c @@ -4008,6 +4008,7 @@ BlockDriverAIOCB *bdrv_aio_discard(BlockDriverState *bs, void bdrv_init(void) { + module_load(MODULE_LOAD_BLOCK); module_call_init(MODULE_INIT_BLOCK); } diff --git a/bsd-user/main.c b/bsd-user/main.c index f9246aa..6cb9e35 100644 --- a/bsd-user/main.c +++ b/bsd-user/main.c @@ -33,6 +33,7 @@ #include "tcg.h" #include "qemu/timer.h" #include "qemu/envlist.h" +#include "qemu/module.h" int singlestep; #if defined(CONFIG_USE_GUEST_BASE) @@ -749,6 +750,8 @@ int main(int argc, char **argv) if (argc <= 1) usage(); + module_load(MODULE_LOAD_UI); + module_load(MODULE_LOAD_NET); module_call_init(MODULE_INIT_QOM); if ((envlist = envlist_create()) == NULL) { diff --git a/configure b/configure index c6d4a62..a80f143 100755 --- a/configure +++ b/configure @@ -198,6 +198,7 @@ datadir="\${prefix}/share" qemu_docdir="\${prefix}/share/doc/qemu" bindir="\${prefix}/bin" libdir="\${prefix}/lib" +moddir="\${prefix}/lib/qemu" libexecdir="\${prefix}/libexec" includedir="\${prefix}/include" sysconfdir="\${prefix}/etc" @@ -673,6 +674,8 @@ for opt do ;; --libdir=*) libdir="$optarg" ;; + --moddir=*) moddir="$optarg" + ;; --libexecdir=*) libexecdir="$optarg" ;; --includedir=*) includedir="$optarg" @@ -1049,6 +1052,7 @@ echo " --datadir=PATH install firmware in PATH$confsuffix" echo " --docdir=PATH install documentation in PATH$confsuffix" echo " --bindir=PATH install binaries in PATH" echo " --libdir=PATH install libraries in PATH" +echo " --moddir=PATH install modules in PATH" echo " --sysconfdir=PATH install config in PATH$confsuffix" echo " --localstatedir=PATH install local state in PATH (set at runtime on win32)" echo " --with-confsuffix=SUFFIX suffix for QEMU data inside datadir and sysconfdir [$confsuffix]" @@ -2252,15 +2256,19 @@ if test "$mingw32" = yes; then else glib_req_ver=2.12 fi -if $pkg_config --atleast-version=$glib_req_ver gthread-2.0; then - glib_cflags=`$pkg_config --cflags gthread-2.0` - glib_libs=`$pkg_config --libs gthread-2.0` - CFLAGS="$glib_cflags $CFLAGS" - LIBS="$glib_libs $LIBS" - libs_qga="$glib_libs $libs_qga" -else - error_exit "glib-$glib_req_ver required to compile QEMU" -fi + +for i in gthread-2.0 gmodule-2.0; do + if $pkg_config --atleast-version=$glib_req_ver $i; then + glib_cflags=`$pkg_config --cflags $i` + glib_libs=`$pkg_config --libs $i` + CFLAGS="$glib_cflags $CFLAGS" + LIBS="$glib_libs $LIBS" + libs_qga="$glib_libs $libs_qga" + else + error_exit "glib-$glib_req_ver required to compile QEMU" + fi +done + ########################################## # pixman support probe @@ -3553,6 +3561,7 @@ echo "Install prefix $prefix" echo "BIOS directory `eval echo $qemu_datadir`" echo "binary directory `eval echo $bindir`" echo "library directory `eval echo $libdir`" +echo "module directory `eval echo $moddir`" echo "libexec directory `eval echo $libexecdir`" echo "include directory `eval echo $includedir`" echo "config directory `eval echo $sysconfdir`" @@ -3675,6 +3684,7 @@ echo all: >> $config_host_mak echo "prefix=$prefix" >> $config_host_mak echo "bindir=$bindir" >> $config_host_mak echo "libdir=$libdir" >> $config_host_mak +echo "moddir=$moddir" >> $config_host_mak echo "libexecdir=$libexecdir" >> $config_host_mak echo "includedir=$includedir" >> $config_host_mak echo "mandir=$mandir" >> $config_host_mak diff --git a/include/qemu/module.h b/include/qemu/module.h index c4ccd57..f00bc25 100644 --- a/include/qemu/module.h +++ b/include/qemu/module.h @@ -37,4 +37,13 @@ void register_module_init(void (*fn)(void), module_init_type type); void module_call_init(module_init_type type); +typedef enum { + MODULE_LOAD_BLOCK = 0, + MODULE_LOAD_UI, + MODULE_LOAD_NET, + MODULE_LOAD_MAX, +} module_load_type; + +void module_load(module_load_type type); + #endif diff --git a/linux-user/main.c b/linux-user/main.c index 5c2f7b2..db08c23 100644 --- a/linux-user/main.c +++ b/linux-user/main.c @@ -34,6 +34,7 @@ #include "qemu/timer.h" #include "qemu/envlist.h" #include "elf.h" +#include <qemu/module.h> char *exec_path; @@ -3551,6 +3552,8 @@ int main(int argc, char **argv, char **envp) int i; int ret; + module_load(MODULE_LOAD_UI); + module_load(MODULE_LOAD_NET); module_call_init(MODULE_INIT_QOM); qemu_cache_utils_init(envp); diff --git a/scripts/create_config b/scripts/create_config index b1adbf5..381564e 100755 --- a/scripts/create_config +++ b/scripts/create_config @@ -26,6 +26,10 @@ case $line in # save for the next definitions prefix=${line#*=} ;; + moddir=*) + eval "moddir=\"${line#*=}\"" + echo "#define CONFIG_MODDIR \"$moddir\"" + ;; CONFIG_AUDIO_DRIVERS=*) drivers=${line#*=} echo "#define CONFIG_AUDIO_DRIVERS \\" @@ -104,6 +108,9 @@ case $line in value=${line#*=} echo "#define $name $value" ;; + DSOSUF=*) + echo "#define HOST_DSOSUF \"${line#*=}\"" + ;; esac done # read diff --git a/util/module.c b/util/module.c index 7acc33d..4d0374a 100644 --- a/util/module.c +++ b/util/module.c @@ -13,6 +13,8 @@ * GNU GPL, version 2 or (at your option) any later version. */ +#include <gmodule.h> +#include <dirent.h> #include "qemu-common.h" #include "qemu/queue.h" #include "qemu/module.h" @@ -79,3 +81,54 @@ void module_call_init(module_init_type type) e->init(); } } + +void module_load(module_load_type type) +{ + const char *path; + const char *dsosuf = HOST_DSOSUF; + char *fname; + int suf_len = strlen(dsosuf); + DIR *dp; + struct dirent *ep = NULL; + GModule *g_module; + + if (!g_module_supported()) { + return; + } + + switch (type) { + case MODULE_LOAD_BLOCK: + path = CONFIG_MODDIR "/block/"; + break; + case MODULE_LOAD_UI: + path = CONFIG_MODDIR "/ui/"; + break; + case MODULE_LOAD_NET: + path = CONFIG_MODDIR "/net/"; + break; + default: + return; + } + + dp = opendir(path); + if (!dp) { + fprintf(stderr, "Failed to open dir %s\n", path); + return; + } + for (ep = readdir(dp); ep; ep = readdir(dp)) { + 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); + } + } +} diff --git a/vl.c b/vl.c index b4b119a..659e53a 100644 --- a/vl.c +++ b/vl.c @@ -2940,6 +2940,8 @@ int main(int argc, char **argv, char **envp) #endif } + module_load(MODULE_LOAD_UI); + module_load(MODULE_LOAD_NET); module_call_init(MODULE_INIT_QOM); qemu_add_opts(&qemu_drive_opts);
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 <famz@redhat.com> --- 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(-)