On Mon, Feb 18, 2013 at 7:53 AM, Lucas De Marchi
<lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> 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.
Fixed.
>> 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.
Whoops, yes.
>> + 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.
These functions are needed in libkmod-module.c. Since the other
private members of the struct kmod_file were exported via functions
like this, it seemed the right thing to do. What would you recommend?
>> +
>> 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 :-)
The downside is that kmod must be built on a host where the syscall
number is known in the kernel headers. The other version would allow
it to build anywhere. I'm fine with this minimal version, regardless.
>
>> /**
>> * 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.
Argh. Mapping added.
>
>> + 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
Thanks for the review! I'll get the next version sent.
-Kees
--
Kees Cook
Chrome OS Security
--
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
