Hi Kees,
On Thu, Jan 31, 2013 at 4:34 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> Hi, just checking on this. Any objections to this patch? I'd like to
> get it into the tree so more people can verify it.
>
> Thanks!
Sorry for the delay. I'm currently on honeymoon and not really paying
attention to my email :-). I just gave it a quick look and I very much
dislike the ifdefs for compat stuff, particularly for including
headers. If we do anything at all, it should not contain that many
ifdefs.
>
> -Kees
>
> On Tue, Jan 22, 2013 at 9:54 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. Also update
>> testsuite to expect the call, and fix callers of mmap() to use NULL
>> instead of 0.
please don't. split the patch so the fix to mmap is separated. Ideally
changes to testsuite should be self-contained as well.
>>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
we don't use s-o-b in kmod
>> ---
>> configure.ac | 1 +
>> libkmod/libkmod-file.c | 16 +++++++++++++++-
>> libkmod/libkmod-index.c | 4 ++--
>> libkmod/libkmod-module.c | 29 +++++++++++++++++++++++++++++
>> libkmod/libkmod-private.h | 2 ++
>> testsuite/init_module.c | 24 ++++++++++++++++++++++++
>> 6 files changed, 73 insertions(+), 3 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index 0f86c25..faab6bd 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -39,6 +39,7 @@ PKG_PROG_PKG_CONFIG
>> #####################################################################
>>
>> AC_CHECK_FUNCS_ONCE(__xstat)
>> +AC_CHECK_FUNCS_ONCE(finit_module)
>>
>> # dietlibc doesn't have st.st_mtim struct member
>> AC_CHECK_MEMBERS([struct stat.st_mtim], [], [], [#include <sys/stat.h>])
>> 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-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;
>> }
>> diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
>> index b1d40b1..31ad5fc 100644
>> --- a/libkmod/libkmod-module.c
>> +++ b/libkmod/libkmod-module.c
>> @@ -762,6 +762,26 @@ KMOD_EXPORT int kmod_module_remove_module(struct kmod_module *mod,
>> }
>>
>> extern long init_module(const void *mem, unsigned long len, const char *args);
>> +#ifdef HAVE_FINIT_MODULE
>> +extern int finit_module(int fd, const char *uargs, int flags);
>> +#else
>> +# include <sys/syscall.h>
>> +# ifndef __NR_finit_module
>> +# if defined(__x86_64__)
>> +# define __NR_finit_module 313
>> +# elif defined(__i386__)
>> +# define __NR_finit_module 350
>> +# elif defined(__arm__)
>> +# define __NR_finit_module 379
>> +# else
>> +# define __NR_finit_module -1
>> +# endif
>> +# endif
these ifdefs are insane. They should go to a separate compat header if
indeed we want them... I actually don't. Really, this doesn't pertain
here.
As I said, I'm currently afk so I didn't give it too much thinking.
I'll be back after Brazilian's carnival (Feb 13).
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
