On Tue, Feb 12, 2013 at 8:32 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> When a module is being loaded directly from disk (no compression,
> etc), pass the file descriptor to the new finit_module() syscall. If
> finit_module is exported by glibc, use it. Otherwise, manually make
> the syscall on architectures where it is known to exist.
> ---
> libkmod/libkmod-file.c | 16 +++++++++++++++-
> libkmod/libkmod-module.c | 18 ++++++++++++++++++
> libkmod/libkmod-private.h | 2 ++
> 3 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
> index ced20a8..76585f5 100644
> --- a/libkmod/libkmod-file.c
> +++ b/libkmod/libkmod-file.c
> @@ -52,6 +52,7 @@ struct kmod_file {
> gzFile gzf;
> #endif
> int fd;
> + int direct;
it's either true or false. It should be bool, not int.
> off_t size;
> void *memory;
> const struct file_ops *ops;
> @@ -254,9 +255,11 @@ static int load_reg(struct kmod_file *file)
> return -errno;
>
> file->size = st.st_size;
> - file->memory = mmap(0, file->size, PROT_READ, MAP_PRIVATE, file->fd, 0);
> + file->memory = mmap(NULL, file->size, PROT_READ, MAP_PRIVATE,
> + file->fd, 0);
> if (file->memory == MAP_FAILED)
> return -errno;
this should be in patch 3.
> + file->direct = 1;
s/1/true/
> return 0;
> }
>
> @@ -300,6 +303,7 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
> magic_size_max = itr->magic_size;
> }
>
> + file->direct = 0;
> if (magic_size_max > 0) {
> char *buf = alloca(magic_size_max + 1);
> ssize_t sz;
> @@ -353,6 +357,16 @@ off_t kmod_file_get_size(const struct kmod_file *file)
> return file->size;
> }
>
> +int kmod_file_get_direct(const struct kmod_file *file)
> +{
> + return file->direct;
> +}
> +
> +int kmod_file_get_fd(const struct kmod_file *file)
> +{
> + return file->fd;
> +}
a leftover from previous patch? there's no reason for this function to exist.
> +
> void kmod_file_unref(struct kmod_file *file)
> {
> if (file->elf)
> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
> index b1d40b1..5a9473a 100644
> --- a/libkmod/libkmod-module.c
> +++ b/libkmod/libkmod-module.c
> @@ -33,6 +33,7 @@
> #include <sys/stat.h>
> #include <sys/types.h>
> #include <sys/mman.h>
> +#include <sys/syscall.h>
> #include <sys/wait.h>
> #include <string.h>
> #include <fnmatch.h>
> @@ -763,6 +764,14 @@ KMOD_EXPORT int kmod_module_remove_module(struct kmod_module *mod,
>
> extern long init_module(const void *mem, unsigned long len, const char *args);
>
> +#ifndef __NR_finit_module
> +# define __NR_finit_module -1
> +#endif
> +static inline int finit_module(int fd, const char *uargs, int flags)
> +{
> + return syscall(__NR_finit_module, fd, uargs, flags);
> +}
> +
this looks much better than in the previous patch :-)
> /**
> * kmod_module_insert_module:
> * @mod: kmod module
> @@ -803,6 +812,14 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
> return err;
> }
>
> + if (kmod_file_get_direct(file)) {
> + err = finit_module(kmod_file_get_fd(file), args,
> + flags & (KMOD_INSERT_FORCE_VERMAGIC |
> + KMOD_INSERT_FORCE_MODVERSION));
This is wrong. We export this flags to users, but they are not the
same flags to pass to kernel. We should map them to
MODULE_INIT_IGNORE_MODVERSIONS and MODULE_INIT_IGNORE_VERMAGIC.
And it's unfortunate that I realized only now that kernel defines are
in the opposite order as we define them.
> + if (err == 0 || errno != ENOSYS)
> + goto init_finished;
> + }
> +
> size = kmod_file_get_size(file);
> mem = kmod_file_get_contents(file);
>
> @@ -829,6 +846,7 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
> }
>
> err = init_module(mem, size, args);
> +init_finished:
> if (err < 0) {
> err = -errno;
> INFO(mod->ctx, "Failed to insert module '%s': %m\n", path);
> diff --git a/libkmod/libkmod-private.h b/libkmod/libkmod-private.h
> index b472c62..c765003 100644
> --- a/libkmod/libkmod-private.h
> +++ b/libkmod/libkmod-private.h
> @@ -142,6 +142,8 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx, const char *filenam
> struct kmod_elf *kmod_file_get_elf(struct kmod_file *file) __attribute__((nonnull(1)));
> void *kmod_file_get_contents(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
> off_t kmod_file_get_size(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
> +int kmod_file_get_direct(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
> +int kmod_file_get_fd(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
As said, this function should not exist or at least should not be
visible to other files.
> void kmod_file_unref(struct kmod_file *file) __attribute__((nonnull(1)));
>
> /* libkmod-elf.c */
> --
> 1.7.9.5
>
Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
