Discussion:
[PATCH 3/3] libkmod: fix arguments to mmap
Kees Cook
2013-02-12 22:32:27 UTC
Permalink
The first argument to mmap should be "NULL" instead of "0".
---
libkmod/libkmod-index.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libkmod/libkmod-index.c b/libkmod/libkmod-index.c
index 516240e..d386f00 100644
--- a/libkmod/libkmod-index.c
+++ b/libkmod/libkmod-index.c
@@ -801,9 +801,9 @@ struct index_mm *index_mm_open(struct kmod_ctx *ctx, const char *filename,
if ((size_t) st.st_size < sizeof(hdr))
goto fail_nommap;

- if ((idx->mm = mmap(0, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0))
+ if ((idx->mm = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0))
== MAP_FAILED) {
- ERR(ctx, "mmap(0, %"PRIu64", PROT_READ, %d, MAP_PRIVATE, 0): %m\n",
+ ERR(ctx, "mmap(NULL, %"PRIu64", PROT_READ, %d, MAP_PRIVATE, 0): %m\n",
st.st_size, fd);
goto fail_nommap;
}
--
1.7.9.5
Kees Cook
2013-02-12 22:32:26 UTC
Permalink
This adds the finit_module logic to the testsuite.
---
testsuite/init_module.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/testsuite/init_module.c b/testsuite/init_module.c
index c4d7efb..ca9f84c 100644
--- a/testsuite/init_module.c
+++ b/testsuite/init_module.c
@@ -28,6 +28,7 @@
#include <stddef.h>
#include <string.h>
#include <stdio.h>
+#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -274,6 +275,29 @@ long init_module(void *mem, unsigned long len, const char *args)
return err;
}

+TS_EXPORT int finit_module(const int fd, const char *args, const int flags);
+
+int finit_module(const int fd, const char *args, const int flags)
+{
+ int err;
+ void *mem;
+ unsigned long len;
+ struct stat st;
+
+ if (fstat(fd, &st) < 0)
+ return -1;
+
+ len = st.st_size;
+ mem = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (mem == MAP_FAILED)
+ return -1;
+
+ err = init_module(mem, len, args);
+ munmap(mem, len);
+
+ return err;
+}
+
/* the test is going away anyway, but lets keep valgrind happy */
void free_resources(void) __attribute__((destructor));
void free_resources(void)
--
1.7.9.5
Lucas De Marchi
2013-02-18 15:25:51 UTC
Permalink
Post by Kees Cook
This adds the finit_module logic to the testsuite.
---
testsuite/init_module.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/testsuite/init_module.c b/testsuite/init_module.c
index c4d7efb..ca9f84c 100644
--- a/testsuite/init_module.c
+++ b/testsuite/init_module.c
@@ -28,6 +28,7 @@
#include <stddef.h>
#include <string.h>
#include <stdio.h>
+#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -274,6 +275,29 @@ long init_module(void *mem, unsigned long len, const char *args)
return err;
}
+TS_EXPORT int finit_module(const int fd, const char *args, const int flags);
+
+int finit_module(const int fd, const char *args, const int flags)
+{
+ int err;
+ void *mem;
+ unsigned long len;
+ struct stat st;
+
+ if (fstat(fd, &st) < 0)
+ return -1;
+
+ len = st.st_size;
+ mem = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (mem == MAP_FAILED)
+ return -1;
+
+ err = init_module(mem, len, args);
+ munmap(mem, len);
+
+ return err;
+}
+
/* the test is going away anyway, but lets keep valgrind happy */
void free_resources(void) __attribute__((destructor));
void free_resources(void)
Looks good, waiting for the update on others to apply this.


Lucas De Marchi
Kees Cook
2013-02-12 22:32:25 UTC
Permalink
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;
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;
+ file->direct = 1;
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;
+}
+
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);
+}
+
/**
* 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));
+ 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)));
void kmod_file_unref(struct kmod_file *file) __attribute__((nonnull(1)));

/* libkmod-elf.c */
--
1.7.9.5
Mike Frysinger
2013-02-18 05:55:42 UTC
Permalink
Post by Kees Cook
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
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);
i think you meant to split this out into patch [3/3]
-mike
Lucas De Marchi
2013-02-18 15:53:15 UTC
Permalink
Post by Kees Cook
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.
Post by Kees Cook
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.
Post by Kees Cook
+ file->direct = 1;
s/1/true/
Post by Kees Cook
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.
Post by Kees Cook
+
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 :-)
Post by Kees Cook
/**
@@ -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.
Post by Kees Cook
+ 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);
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.
Post by Kees Cook
void kmod_file_unref(struct kmod_file *file) __attribute__((nonnull(1)));
/* libkmod-elf.c */
--
1.7.9.5
Lucas De Marchi
Kees Cook
2013-02-18 19:41:20 UTC
Permalink
On Mon, Feb 18, 2013 at 7:53 AM, Lucas De Marchi
Post by Lucas De Marchi
Post by Kees Cook
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.
Post by Lucas De Marchi
Post by Kees Cook
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.
Post by Lucas De Marchi
Post by Kees Cook
+ file->direct = 1;
s/1/true/
Post by Kees Cook
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?
Post by Lucas De Marchi
Post by Kees Cook
+
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.
Post by Lucas De Marchi
Post by Kees Cook
/**
@@ -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.
Post by Lucas De Marchi
Post by Kees Cook
+ 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);
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.
Post by Kees Cook
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
Lucas De Marchi
2013-02-18 20:02:14 UTC
Permalink
Post by Kees Cook
On Mon, Feb 18, 2013 at 7:53 AM, Lucas De Marchi
Post by Lucas De Marchi
Post by Kees Cook
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.
Post by Lucas De Marchi
Post by Kees Cook
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.
Post by Lucas De Marchi
Post by Kees Cook
+ file->direct = 1;
s/1/true/
Post by Kees Cook
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?
err, my bad: I didn't see kmod_file_get_fd() was actually being used.

Lucas De Marchi
Lucas De Marchi
2013-02-18 14:55:06 UTC
Permalink
Post by Kees Cook
The first argument to mmap should be "NULL" instead of "0".
---
libkmod/libkmod-index.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/libkmod/libkmod-index.c b/libkmod/libkmod-index.c
index 516240e..d386f00 100644
--- a/libkmod/libkmod-index.c
+++ b/libkmod/libkmod-index.c
@@ -801,9 +801,9 @@ struct index_mm *index_mm_open(struct kmod_ctx *ctx, const char *filename,
if ((size_t) st.st_size < sizeof(hdr))
goto fail_nommap;
- if ((idx->mm = mmap(0, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0))
+ if ((idx->mm = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0))
== MAP_FAILED) {
- ERR(ctx, "mmap(0, %"PRIu64", PROT_READ, %d, MAP_PRIVATE, 0): %m\n",
+ ERR(ctx, "mmap(NULL, %"PRIu64", PROT_READ, %d, MAP_PRIVATE, 0): %m\n",
st.st_size, fd);
goto fail_nommap;
}
--
Please correct the split of these patches and bring the change in the
first patch to this one. Otherwise looks good.


Lucas De Marchi
Kees Cook
2013-02-18 20:02:33 UTC
Permalink
This adds the finit_module logic to the testsuite.
---
testsuite/init_module.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/testsuite/init_module.c b/testsuite/init_module.c
index c4d7efb..ca9f84c 100644
--- a/testsuite/init_module.c
+++ b/testsuite/init_module.c
@@ -28,6 +28,7 @@
#include <stddef.h>
#include <string.h>
#include <stdio.h>
+#include <sys/mman.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>
@@ -274,6 +275,29 @@ long init_module(void *mem, unsigned long len, const char *args)
return err;
}

+TS_EXPORT int finit_module(const int fd, const char *args, const int flags);
+
+int finit_module(const int fd, const char *args, const int flags)
+{
+ int err;
+ void *mem;
+ unsigned long len;
+ struct stat st;
+
+ if (fstat(fd, &st) < 0)
+ return -1;
+
+ len = st.st_size;
+ mem = mmap(NULL, len, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (mem == MAP_FAILED)
+ return -1;
+
+ err = init_module(mem, len, args);
+ munmap(mem, len);
+
+ return err;
+}
+
/* the test is going away anyway, but lets keep valgrind happy */
void free_resources(void) __attribute__((destructor));
void free_resources(void)
--
1.7.9.5
Kees Cook
2013-02-18 20:02:32 UTC
Permalink
When a module is being loaded directly from disk (no compression, etc),
pass the file descriptor to the new finit_module() syscall. If the
finit_module syscall is exported by the kernel syscall headers, use it.
Additionally, if the kernel's module.h file is available, map kmod flags
to finit_module flags.
---
configure.ac | 3 +++
libkmod/libkmod-file.c | 13 +++++++++++++
libkmod/libkmod-module.c | 29 +++++++++++++++++++++++++++++
libkmod/libkmod-private.h | 2 ++
4 files changed, 47 insertions(+)

diff --git a/configure.ac b/configure.ac
index 0f86c25..566b317 100644
--- a/configure.ac
+++ b/configure.ac
@@ -43,6 +43,9 @@ AC_CHECK_FUNCS_ONCE(__xstat)
# dietlibc doesn't have st.st_mtim struct member
AC_CHECK_MEMBERS([struct stat.st_mtim], [], [], [#include <sys/stat.h>])

+# Check kernel headers
+AC_CHECK_HEADERS_ONCE([linux/module.h])
+

#####################################################################
# --with-
diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index ced20a8..219c63b 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -52,6 +52,7 @@ struct kmod_file {
gzFile gzf;
#endif
int fd;
+ bool direct;
off_t size;
void *memory;
const struct file_ops *ops;
@@ -257,6 +258,7 @@ static int load_reg(struct kmod_file *file)
file->memory = mmap(0, file->size, PROT_READ, MAP_PRIVATE, file->fd, 0);
if (file->memory == MAP_FAILED)
return -errno;
+ file->direct = true;
return 0;
}

@@ -300,6 +302,7 @@ struct kmod_file *kmod_file_open(const struct kmod_ctx *ctx,
magic_size_max = itr->magic_size;
}

+ file->direct = false;
if (magic_size_max > 0) {
char *buf = alloca(magic_size_max + 1);
ssize_t sz;
@@ -353,6 +356,16 @@ off_t kmod_file_get_size(const struct kmod_file *file)
return file->size;
}

+bool 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;
+}
+
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..7b38e64 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -33,10 +33,15 @@
#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>

+#ifdef HAVE_LINUX_MODULE_H
+#include <linux/module.h>
+#endif
+
#include "libkmod.h"
#include "libkmod-private.h"

@@ -763,6 +768,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);
+}
+
/**
* kmod_module_insert_module:
* @mod: kmod module
@@ -803,6 +816,21 @@ KMOD_EXPORT int kmod_module_insert_module(struct kmod_module *mod,
return err;
}

+ if (kmod_file_get_direct(file)) {
+ unsigned int kernel_flags = 0;
+
+#ifdef HAVE_LINUX_MODULE_H
+ if (flags & KMOD_INSERT_FORCE_VERMAGIC)
+ kernel_flags |= MODULE_INIT_IGNORE_VERMAGIC;
+ if (flags & KMOD_INSERT_FORCE_MODVERSION)
+ kernel_flags |= MODULE_INIT_IGNORE_MODVERSIONS;
+#endif
+
+ err = finit_module(kmod_file_get_fd(file), args, kernel_flags);
+ if (err == 0 || errno != ENOSYS)
+ goto init_finished;
+ }
+
size = kmod_file_get_size(file);
mem = kmod_file_get_contents(file);

@@ -829,6 +857,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..7748b14 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)));
+bool 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)));
void kmod_file_unref(struct kmod_file *file) __attribute__((nonnull(1)));

/* libkmod-elf.c */
--
1.7.9.5
Kees Cook
2013-02-18 20:02:34 UTC
Permalink
The first argument to mmap should be "NULL" instead of "0".
---
libkmod/libkmod-file.c | 3 ++-
libkmod/libkmod-index.c | 4 ++--
2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/libkmod/libkmod-file.c b/libkmod/libkmod-file.c
index 219c63b..5313796 100644
--- a/libkmod/libkmod-file.c
+++ b/libkmod/libkmod-file.c
@@ -255,7 +255,8 @@ 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;
file->direct = true;
diff --git a/libkmod/libkmod-index.c b/libkmod/libkmod-index.c
index 516240e..d386f00 100644
--- a/libkmod/libkmod-index.c
+++ b/libkmod/libkmod-index.c
@@ -801,9 +801,9 @@ struct index_mm *index_mm_open(struct kmod_ctx *ctx, const char *filename,
if ((size_t) st.st_size < sizeof(hdr))
goto fail_nommap;

- if ((idx->mm = mmap(0, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0))
+ if ((idx->mm = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0))
== MAP_FAILED) {
- ERR(ctx, "mmap(0, %"PRIu64", PROT_READ, %d, MAP_PRIVATE, 0): %m\n",
+ ERR(ctx, "mmap(NULL, %"PRIu64", PROT_READ, %d, MAP_PRIVATE, 0): %m\n",
st.st_size, fd);
goto fail_nommap;
}
--
1.7.9.5
Lucas De Marchi
2013-02-19 22:22:11 UTC
Permalink
This implements the logic for using finit_module when it is available
and supported. The series includes the core logic, the testsuite update,
and some additional clean-ups identified during the review process.
- Use "bool" for direct-load state tracking.
- Detect linux module uapi and map kmod flags to kernel flags.
- Correctly split NULL-arg mmap patch.
Thanks!
All patches have been applied.

Thanks
Lucas De Marchi

Loading...