subject:"\[PATCH 1\/4\] module\: add syscall to load module from fd"

On Sun, Jan 6, 2013 at 9:24 PM, Kees Cook  wrote:
> On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
>  wrote:
>> Hi Rusty, (and Lucas, and Kees)
>>
>> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell  wrote:
>>> Michael Kerrisk  writes:
 Hi Rusty,
>>>
>>> Hi Michael,
>>>
 The description here is rather thin. Could you supply a sentence or
 two for each of MODULE_INIT_IGNORE_MODVERSIONS and
 MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
 page?

 Thanks,
>>>
>>> There are one or two safety checks built into a module, which are
>>> checked to match the kernel on module load.  The first is a "vermagic"
>>> string containing the kernel version number and prominent features (such
>>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>>> version hash is recorded for each symbol the module uses based on the
>>> types it refers to: in this case, the kernel version number within the
>>> "vermagic" string is ignored, as the symbol version hashes are assumed
>>> to be sufficiently reliable.
>>>
>>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>>> that the version hashes are to be ignored.  If the kernel is built to
>>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>>> loading will continue, otherwise it will fail with ENOEXEC as expected
>>> for malformed modules.
>>>
>>> Hope that is more usable?
>>
>> Yes, that helps. I did some reworking of that text. Hopefully, I did
>> not introduce any errors.
>>
>> Below is the text that is proposed to document finit_module() in the
>> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)
>
> Looks good to me!
>
> Reviewed-by: Kees Cook 

Thanks Kees!



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2013-01-06 Thread Kees Cook

On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
 wrote:
> Hi Rusty, (and Lucas, and Kees)
>
> On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell  wrote:
>> Michael Kerrisk  writes:
>>> Hi Rusty,
>>
>> Hi Michael,
>>
>>> The description here is rather thin. Could you supply a sentence or
>>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>>> page?
>>>
>>> Thanks,
>>
>> There are one or two safety checks built into a module, which are
>> checked to match the kernel on module load.  The first is a "vermagic"
>> string containing the kernel version number and prominent features (such
>> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
>> version hash is recorded for each symbol the module uses based on the
>> types it refers to: in this case, the kernel version number within the
>> "vermagic" string is ignored, as the symbol version hashes are assumed
>> to be sufficiently reliable.
>>
>> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
>> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
>> that the version hashes are to be ignored.  If the kernel is built to
>> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
>> loading will continue, otherwise it will fail with ENOEXEC as expected
>> for malformed modules.
>>
>> Hope that is more usable?
>
> Yes, that helps. I did some reworking of that text. Hopefully, I did
> not introduce any errors.
>
> Below is the text that is proposed to document finit_module() in the
> man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Looks good to me!

Reviewed-by: Kees Cook 

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

Hi Rusty, (and Lucas, and Kees)

On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell  wrote:
> Michael Kerrisk  writes:
>> Hi Rusty,
>
> Hi Michael,
>
>> The description here is rather thin. Could you supply a sentence or
>> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
>> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
>> page?
>>
>> Thanks,
>
> There are one or two safety checks built into a module, which are
> checked to match the kernel on module load.  The first is a "vermagic"
> string containing the kernel version number and prominent features (such
> as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
> version hash is recorded for each symbol the module uses based on the
> types it refers to: in this case, the kernel version number within the
> "vermagic" string is ignored, as the symbol version hashes are assumed
> to be sufficiently reliable.
>
> Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
> is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
> that the version hashes are to be ignored.  If the kernel is built to
> permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
> loading will continue, otherwise it will fail with ENOEXEC as expected
> for malformed modules.
>
> Hope that is more usable?

Yes, that helps. I did some reworking of that text. Hopefully, I did
not introduce any errors.

Below is the text that is proposed to document finit_module() in the
man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Thanks,

Michael

   finit_module()
   The finit_module() system call is like init_module(), but reads
   the module to be loaded from the file  descriptor  fd.   It  is
   useful  when  the authenticity of a kernel module can be deter‐
   mined from its location in the file system; in cases where that
   is  possible,  the  overhead  of using cryptographically signed
   modules to determine  the  authenticity  of  a  module  can  be
   avoided.  The param_values argument is as for init_module().

   The  flags  argument  modifies the operation of finit_module().
   It is a bit mask value created by ORing together zero  or  more
   of the following flags:

   MODULE_INIT_IGNORE_MODVERSIONS
  Ignore symbol version hashes.

   MODULE_INIT_IGNORE_VERMAGIC
  Ignore kernel version magic.

   There are some safety checks built into a module to ensure that
   it matches the kernel against which it is loaded.  These checks
   are  recorded  when  the  module is built and verified when the
   module is loaded.   First,  the  module  records  a  "vermagic"
   string  containing the kernel version number and prominent fea‐
   tures (such as the CPU type).  Second, if the module was  built
   with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
   version hash is recorded for each symbol the module uses.  This
   hash  is  based  on the types of the arguments and return value
   for the function named by the symbol.  In this case, the kernel
   version  number within the "vermagic" string is ignored, as the
   symbol version hashes are assumed to be sufficiently reliable.

   Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
   "vermagic"   string   is   to   be   ignored,   and   the  MOD‐
   ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
   sion  hashes are to be ignored.  If the kernel is built to per‐
   mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
   ULE_FORCE_LOAD),  then loading will continue, otherwise it will
   fail with ENOEXEC as expected for malformed modules.
...
   ERRORS
...
   The following errors may additionally occur for finit_module():

   EBADF  The file referred to by fd is not opened for reading.

   EFBIG  The file referred to by fd is too large.

   EINVAL flags is invalid.

   ENOEXEC
  fd does not refer to an open file.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

Hi Rusty, (and Lucas, and Kees)

On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk mtk.manpa...@gmail.com writes:
 Hi Rusty,

 Hi Michael,

 The description here is rather thin. Could you supply a sentence or
 two for each of MODULE_INIT_IGNORE_MODVERSIONS and
 MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
 page?

 Thanks,

 There are one or two safety checks built into a module, which are
 checked to match the kernel on module load.  The first is a vermagic
 string containing the kernel version number and prominent features (such
 as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
 version hash is recorded for each symbol the module uses based on the
 types it refers to: in this case, the kernel version number within the
 vermagic string is ignored, as the symbol version hashes are assumed
 to be sufficiently reliable.

 Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
 is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
 that the version hashes are to be ignored.  If the kernel is built to
 permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
 loading will continue, otherwise it will fail with ENOEXEC as expected
 for malformed modules.

 Hope that is more usable?

Yes, that helps. I did some reworking of that text. Hopefully, I did
not introduce any errors.

Below is the text that is proposed to document finit_module() in the
man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Thanks,

Michael

   finit_module()
   The finit_module() system call is like init_module(), but reads
   the module to be loaded from the file  descriptor  fd.   It  is
   useful  when  the authenticity of a kernel module can be deter‐
   mined from its location in the file system; in cases where that
   is  possible,  the  overhead  of using cryptographically signed
   modules to determine  the  authenticity  of  a  module  can  be
   avoided.  The param_values argument is as for init_module().

   The  flags  argument  modifies the operation of finit_module().
   It is a bit mask value created by ORing together zero  or  more
   of the following flags:

   MODULE_INIT_IGNORE_MODVERSIONS
  Ignore symbol version hashes.

   MODULE_INIT_IGNORE_VERMAGIC
  Ignore kernel version magic.

   There are some safety checks built into a module to ensure that
   it matches the kernel against which it is loaded.  These checks
   are  recorded  when  the  module is built and verified when the
   module is loaded.   First,  the  module  records  a  vermagic
   string  containing the kernel version number and prominent fea‐
   tures (such as the CPU type).  Second, if the module was  built
   with  the  CONFIG_MODVERSIONS  configuration  option enabled, a
   version hash is recorded for each symbol the module uses.  This
   hash  is  based  on the types of the arguments and return value
   for the function named by the symbol.  In this case, the kernel
   version  number within the vermagic string is ignored, as the
   symbol version hashes are assumed to be sufficiently reliable.

   Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates  that  the
   vermagic   string   is   to   be   ignored,   and   the  MOD‐
   ULE_INIT_IGNORE_MODVERSIONS flag indicates that the symbol ver‐
   sion  hashes are to be ignored.  If the kernel is built to per‐
   mit  forced  loading   (i.e.,   configured   with   CONFIG_MOD‐
   ULE_FORCE_LOAD),  then loading will continue, otherwise it will
   fail with ENOEXEC as expected for malformed modules.
...
   ERRORS
...
   The following errors may additionally occur for finit_module():

   EBADF  The file referred to by fd is not opened for reading.

   EFBIG  The file referred to by fd is too large.

   EINVAL flags is invalid.

   ENOEXEC
  fd does not refer to an open file.


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of The Linux Programming Interface; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2013-01-06 Thread Kees Cook

On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
mtk.manpa...@gmail.com wrote:
 Hi Rusty, (and Lucas, and Kees)

 On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk mtk.manpa...@gmail.com writes:
 Hi Rusty,

 Hi Michael,

 The description here is rather thin. Could you supply a sentence or
 two for each of MODULE_INIT_IGNORE_MODVERSIONS and
 MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
 page?

 Thanks,

 There are one or two safety checks built into a module, which are
 checked to match the kernel on module load.  The first is a vermagic
 string containing the kernel version number and prominent features (such
 as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
 version hash is recorded for each symbol the module uses based on the
 types it refers to: in this case, the kernel version number within the
 vermagic string is ignored, as the symbol version hashes are assumed
 to be sufficiently reliable.

 Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
 is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
 that the version hashes are to be ignored.  If the kernel is built to
 permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
 loading will continue, otherwise it will fail with ENOEXEC as expected
 for malformed modules.

 Hope that is more usable?

 Yes, that helps. I did some reworking of that text. Hopefully, I did
 not introduce any errors.

 Below is the text that is proposed to document finit_module() in the
 man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

Looks good to me!

Reviewed-by: Kees Cook keesc...@chromium.org

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Sun, Jan 6, 2013 at 9:24 PM, Kees Cook keesc...@chromium.org wrote:
 On Sun, Jan 6, 2013 at 11:59 AM, Michael Kerrisk (man-pages)
 mtk.manpa...@gmail.com wrote:
 Hi Rusty, (and Lucas, and Kees)

 On Thu, Jan 3, 2013 at 1:12 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk mtk.manpa...@gmail.com writes:
 Hi Rusty,

 Hi Michael,

 The description here is rather thin. Could you supply a sentence or
 two for each of MODULE_INIT_IGNORE_MODVERSIONS and
 MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
 page?

 Thanks,

 There are one or two safety checks built into a module, which are
 checked to match the kernel on module load.  The first is a vermagic
 string containing the kernel version number and prominent features (such
 as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
 version hash is recorded for each symbol the module uses based on the
 types it refers to: in this case, the kernel version number within the
 vermagic string is ignored, as the symbol version hashes are assumed
 to be sufficiently reliable.

 Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
 is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
 that the version hashes are to be ignored.  If the kernel is built to
 permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
 loading will continue, otherwise it will fail with ENOEXEC as expected
 for malformed modules.

 Hope that is more usable?

 Yes, that helps. I did some reworking of that text. Hopefully, I did
 not introduce any errors.

 Below is the text that is proposed to document finit_module() in the
 man pages. I'd appreciate any review (Kees, Lucas, Rusty?)

 Looks good to me!

 Reviewed-by: Kees Cook keesc...@chromium.org

Thanks Kees!



-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of The Linux Programming Interface; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2013-01-02 Thread Rusty Russell

Michael Kerrisk  writes:
> Hi Rusty,

Hi Michael,

> The description here is rather thin. Could you supply a sentence or
> two for each of MODULE_INIT_IGNORE_MODVERSIONS and
> MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
> page?
>
> Thanks,

There are one or two safety checks built into a module, which are
checked to match the kernel on module load.  The first is a "vermagic"
string containing the kernel version number and prominent features (such
as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
version hash is recorded for each symbol the module uses based on the
types it refers to: in this case, the kernel version number within the
"vermagic" string is ignored, as the symbol version hashes are assumed
to be sufficiently reliable.

Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
that the version hashes are to be ignored.  If the kernel is built to
permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
loading will continue, otherwise it will fail with ENOEXEC as expected
for malformed modules.

Hope that is more usable?

Thanks,
Rusty,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2013-01-02 Thread Rusty Russell

Michael Kerrisk mtk.manpa...@gmail.com writes:
 Hi Rusty,

Hi Michael,

 The description here is rather thin. Could you supply a sentence or
 two for each of MODULE_INIT_IGNORE_MODVERSIONS and
 MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
 page?

 Thanks,

There are one or two safety checks built into a module, which are
checked to match the kernel on module load.  The first is a vermagic
string containing the kernel version number and prominent features (such
as CPU type).  If the module was built with CONFIG_MODVERSIONS set, a
version hash is recorded for each symbol the module uses based on the
types it refers to: in this case, the kernel version number within the
vermagic string is ignored, as the symbol version hashes are assumed
to be sufficiently reliable.

Using the MODULE_INIT_IGNORE_VERMAGIC flag indicates that the vermagic
is to be ignored, and the MODULE_INIT_IGNORE_MODVERSIONS flag indicates
that the version hashes are to be ignored.  If the kernel is built to
permit such forced loading (ie. CONFIG_MODULE_FORCE_LOAD is set) then
loading will continue, otherwise it will fail with ENOEXEC as expected
for malformed modules.

Hope that is more usable?

Thanks,
Rusty,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-12-20 Thread Michael Kerrisk

Hi Rusty,

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk 
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?
>
> Thanks,
> Rusty.
>
> module: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
> useful for eliminating the only case where kmod has to mangle a module's
> internals: for overriding module versioning.

The description here is rather thin. Could you supply a sentence or
two for each of MODULE_INIT_IGNORE_MODVERSIONS and
MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
page?

Thanks,

Michael


>
> Signed-off-by: Rusty Russell 
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 32bc035..8cf7b50 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
>
>  asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
>  unsigned long idx1, unsigned long idx2);
> -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
> +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int 
> flags);
>  #endif
> diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
> new file mode 100644
> index 000..38da425
> --- /dev/null
> +++ b/include/uapi/linux/module.h
> @@ -0,0 +1,8 @@
> +#ifndef _UAPI_LINUX_MODULE_H
> +#define _UAPI_LINUX_MODULE_H
> +
> +/* Flags for sys_finit_module: */
> +#define MODULE_INIT_IGNORE_MODVERSIONS 1
> +#define MODULE_INIT_IGNORE_VERMAGIC2
> +
> +#endif /* _UAPI_LINUX_MODULE_H */
> diff --git a/kernel/module.c b/kernel/module.c
> index 261bf82..55b49cd 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -61,6 +61,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "module-internal.h"
>
>  #define CREATE_TRACE_POINTS
> @@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
> vfree(info->hdr);
>  }
>
> -static int rewrite_section_headers(struct load_info *info)
> +static int rewrite_section_headers(struct load_info *info, int flags)
>  {
> unsigned int i;
>
> @@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info 
> *info)
> }
>
> /* Track but don't keep modinfo and version sections. */
> -   info->index.vers = find_sec(info, "__versions");
> +   if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
> +   info->index.vers = 0; /* Pretend no __versions section! */
> +   else
> +   info->index.vers = find_sec(info, "__versions");
> info->index.info = find_sec(info, ".modinfo");
> info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
> info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
> @@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info 
> *info)
>   * Return the temporary module pointer (we'll replace it with the final
>   * one when we move the module sections around).
>   */
> -static struct module *setup_load_info(struct load_info *info)
> +static struct module *setup_load_info(struct load_info *info, int flags)
>  {
> unsigned int i;
> int err;
> @@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info 
> *info)
> info->secstrings = (void *)info->hdr
> + info->sechdrs[info->hdr->e_shstrndx].sh_offset;
>
> -   err = rewrite_section_headers(info);
> +   err = rewrite_section_headers(info, flags);
> if (err)
> return ERR_PTR(err);
>
> @@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct 
> load_info *info)
> return mod;
>  }
>
> -static int check_modinfo(struct module *mod, struct load_info *info)
> +static int check_modinfo(struct module *mod, struct load_info *info, int 
> flags)
>  {
> const char *modmagic = get_modinfo(info, "vermagic");
> int err;
>
> +   if (flags & MODULE_INIT_IGNORE_VERMAGIC)
> +   modmagic = NULL;
> +
> /* This is allowed: modprobe --force will invalidate it. */
> if (!modmagic) {
> err = try_to_force_load(mod, "bad vermagic");
> @@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
> return 0;
>  }
>
> -static struct module *layout_and_allocate(struct load_info *info)
> +static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
> /* Module within temporary copy. */
> struct module *mod;
> Elf_Shdr *pcpusec;
> int err;
>
> -   mod = setup_load_info(info);
> +   mod = setup_load_info(info, flags);
> if

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-12-20 Thread Michael Kerrisk

Hi Rusty,

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

The description here is rather thin. Could you supply a sentence or
two for each of MODULE_INIT_IGNORE_MODVERSIONS and
MODULE_INIT_IGNORE_VERMAGIC that would be suitable for the manual
page?

Thanks,

Michael



 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

 diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
 index 32bc035..8cf7b50 100644
 --- a/include/linux/syscalls.h
 +++ b/include/linux/syscalls.h
 @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,

  asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
  unsigned long idx1, unsigned long idx2);
 -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int 
 flags);
  #endif
 diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
 new file mode 100644
 index 000..38da425
 --- /dev/null
 +++ b/include/uapi/linux/module.h
 @@ -0,0 +1,8 @@
 +#ifndef _UAPI_LINUX_MODULE_H
 +#define _UAPI_LINUX_MODULE_H
 +
 +/* Flags for sys_finit_module: */
 +#define MODULE_INIT_IGNORE_MODVERSIONS 1
 +#define MODULE_INIT_IGNORE_VERMAGIC2
 +
 +#endif /* _UAPI_LINUX_MODULE_H */
 diff --git a/kernel/module.c b/kernel/module.c
 index 261bf82..55b49cd 100644
 --- a/kernel/module.c
 +++ b/kernel/module.c
 @@ -61,6 +61,7 @@
  #include linux/pfn.h
  #include linux/bsearch.h
  #include linux/fips.h
 +#include uapi/linux/module.h
  #include module-internal.h

  #define CREATE_TRACE_POINTS
 @@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
 vfree(info-hdr);
  }

 -static int rewrite_section_headers(struct load_info *info)
 +static int rewrite_section_headers(struct load_info *info, int flags)
  {
 unsigned int i;

 @@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info 
 *info)
 }

 /* Track but don't keep modinfo and version sections. */
 -   info-index.vers = find_sec(info, __versions);
 +   if (flags  MODULE_INIT_IGNORE_MODVERSIONS)
 +   info-index.vers = 0; /* Pretend no __versions section! */
 +   else
 +   info-index.vers = find_sec(info, __versions);
 info-index.info = find_sec(info, .modinfo);
 info-sechdrs[info-index.info].sh_flags = ~(unsigned long)SHF_ALLOC;
 info-sechdrs[info-index.vers].sh_flags = ~(unsigned long)SHF_ALLOC;
 @@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info 
 *info)
   * Return the temporary module pointer (we'll replace it with the final
   * one when we move the module sections around).
   */
 -static struct module *setup_load_info(struct load_info *info)
 +static struct module *setup_load_info(struct load_info *info, int flags)
  {
 unsigned int i;
 int err;
 @@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info 
 *info)
 info-secstrings = (void *)info-hdr
 + info-sechdrs[info-hdr-e_shstrndx].sh_offset;

 -   err = rewrite_section_headers(info);
 +   err = rewrite_section_headers(info, flags);
 if (err)
 return ERR_PTR(err);

 @@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct 
 load_info *info)
 return mod;
  }

 -static int check_modinfo(struct module *mod, struct load_info *info)
 +static int check_modinfo(struct module *mod, struct load_info *info, int 
 flags)
  {
 const char *modmagic = get_modinfo(info, vermagic);
 int err;

 +   if (flags  MODULE_INIT_IGNORE_VERMAGIC)
 +   modmagic = NULL;
 +
 /* This is allowed: modprobe --force will invalidate it. */
 if (!modmagic) {
 err = try_to_force_load(mod, bad vermagic);
 @@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
 return 0;
  }

 -static struct module *layout_and_allocate(struct load_info *info)
 +static struct module *layout_and_allocate(struct load_info *info, int flags)
  {
 /* Module within temporary copy. */
 struct module *mod;
 Elf_Shdr *pcpusec;
 int err;

 -   mod = setup_load_info(info);
 +   mod = setup_load_info(info, flags);
 if (IS_ERR(mod))
 return

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-31 Thread Rusty Russell

Kees Cook  writes:
> Rusty,
>
> I haven't seen this land in your modules-next tree. I just wanted to
> make sure it hadn't gotten lost. I'd like to do some kmod tests
> against linux-next, but I've been waiting for this to appear.

Yes, sorting that out now, they should be in tomorrow's linux-next.
And I've sent the ppc patch to linuxppc-dev for Acks.

Thanks for the prod,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-31 Thread Rusty Russell

Kees Cook keesc...@chromium.org writes:
 Rusty,

 I haven't seen this land in your modules-next tree. I just wanted to
 make sure it hadn't gotten lost. I'd like to do some kmod tests
 against linux-next, but I've been waiting for this to appear.

Yes, sorting that out now, they should be in tomorrow's linux-next.
And I've sent the ppc patch to linuxppc-dev for Acks.

Thanks for the prod,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-30 Thread Kees Cook

On Mon, Oct 22, 2012 at 12:39 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk 
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?
>
> Thanks,
> Rusty.
>
> module: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
> useful for eliminating the only case where kmod has to mangle a module's
> internals: for overriding module versioning.
>
> Signed-off-by: Rusty Russell 

Rusty,

I haven't seen this land in your modules-next tree. I just wanted to
make sure it hadn't gotten lost. I'd like to do some kmod tests
against linux-next, but I've been waiting for this to appear.

I acked this before, but as long as I'm replying again:

Acked-by: Kees Cook 

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-30 Thread Kees Cook

On Mon, Oct 22, 2012 at 12:39 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

Rusty,

I haven't seen this land in your modules-next tree. I just wanted to
make sure it hadn't gotten lost. I'd like to do some kmod tests
against linux-next, but I've been waiting for this to appear.

I acked this before, but as long as I'm replying again:

Acked-by: Kees Cook keesc...@chromium.org

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-24 Thread Rusty Russell

Lucas De Marchi  writes:
> On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook  wrote:
>> On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
>>  wrote:
>>> sure... but do you realize this will fail in case kernel is checking
>>> module signature and we passed --force to modprobe (therefore mangled
>>> the decompressed memory area)?
>>
>> Hm, yeah, userspace mangling of a module plus signing would fail.
>> Seems like mangling and signing aren't compatible. Doing it in
>> kernel-space (as now written for finit_module) solves that, but it
>> means that now compression isn't possible if you need both signing and
>> mangling.

Signing and mangling are always incompatible, of course.

Compressed modules breaks Kees' (and IMA's) requirement to have an fd
attached, unless the kernel does the decompression.  We have that code
already, in fact.

It would be easy to add a config option the recognize the compression
magic and uncompress in-kernel.  That would happen after the signature
check, of course, and Just Work.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-24 Thread Rusty Russell

Lucas De Marchi lucas.demar...@profusion.mobi writes:
 On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook keesc...@chromium.org wrote:
 On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
 lucas.demar...@profusion.mobi wrote:
 sure... but do you realize this will fail in case kernel is checking
 module signature and we passed --force to modprobe (therefore mangled
 the decompressed memory area)?

 Hm, yeah, userspace mangling of a module plus signing would fail.
 Seems like mangling and signing aren't compatible. Doing it in
 kernel-space (as now written for finit_module) solves that, but it
 means that now compression isn't possible if you need both signing and
 mangling.

Signing and mangling are always incompatible, of course.

Compressed modules breaks Kees' (and IMA's) requirement to have an fd
attached, unless the kernel does the decompression.  We have that code
already, in fact.

It would be easy to add a config option the recognize the compression
magic and uncompress in-kernel.  That would happen after the signature
check, of course, and Just Work.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread Lucas De Marchi

On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook  wrote:
> On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
>  wrote:
>> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook  wrote:
>>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>>>  wrote:
 On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell  
 wrote:
> "Michael Kerrisk (man-pages)"  writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk 
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?
>
> Thanks,
> Rusty.
>
> module: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
> useful for eliminating the only case where kmod has to mangle a module's
> internals: for overriding module versioning.
>
> Signed-off-by: Rusty Russell 
>>>
>>> Acked-by: Kees Cook 
>>>
 I wonder if we shouldn't get a new init_module2() as well, adding the
 flags parameter. Of course this would be in another patch.

 My worries are that for compressed modules we still need to use
 init_module() and then --force won't work with signed modules.
>>>
>>> For those cases, I think it should remain up to userspace to do the
>>> decompress and use init_module(). The code I'd written for patching
>>> module-init-tools basically just kept the fd around if it didn't need
>>> to mangle the module, and it would use finit_module (written before
>>> the flags argument was added):
>>>
>>> /* request kernel linkage */
>>> -   ret = init_module(module->data, module->len, opts);
>>> +   if (fd < 0)
>>> +   ret = init_module(module->data, module->len, opts);
>>> +   else {
>>> +   ret = finit_module(fd, opts);
>>> +   if (ret != 0 && errno == ENOSYS)
>>> +   ret = init_module(module->data, module->len, opts);
>>> +   }
>>> if (ret != 0) {
>>>
>>> (And yes, I realize kmod is what'll actually be getting this logic.
>>> This was for my testing in Chrome OS, which is still using
>>> module-init-tools.)
>>
>> sure... but do you realize this will fail in case kernel is checking
>> module signature and we passed --force to modprobe (therefore mangled
>> the decompressed memory area)?
>
> Hm, yeah, userspace mangling of a module plus signing would fail.
> Seems like mangling and signing aren't compatible. Doing it in
> kernel-space (as now written for finit_module) solves that, but it
> means that now compression isn't possible if you need both signing and
> mangling.
>
> I'm not a user of signing, compression, or mangling, so I'm probably a
> bit unimaginative here. It seems like the case for needing all three
> is pretty uncommon. (e.g. if you're doing compression, you're probably
> building embedded images, which means you're unlikely to need
> --force.)


Some desktop distros ship compressed modules by default. I received
feedback from distros some months ago that this is basically because
of the disk space, not performance.  However some measurements I did
in a regular laptop with spinning disk showed a small advantage
performance-wise, too  (with SSD is another story and uncompressed
wins by a large margin)

Since this only affects users of --force option, I think it only
affects module developers, who could uncompress the module, call
depmod again, and modprobe it. ( Mixing compressed and uncompressed
modules used not to work in module-init-tools and earlier versions of
kmod wrt dependencies, but now it should be a seamless operation )


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread H. Peter Anvin


On 10/23/2012 08:42 AM, Kees Cook wrote:


Hm, yeah, userspace mangling of a module plus signing would fail.
Seems like mangling and signing aren't compatible. Doing it in
kernel-space (as now written for finit_module) solves that, but it
means that now compression isn't possible if you need both signing and
mangling.

I'm not a user of signing, compression, or mangling, so I'm probably a
bit unimaginative here. It seems like the case for needing all three
is pretty uncommon. (e.g. if you're doing compression, you're probably
building embedded images, which means you're unlikely to need
--force.)



In particular, mangling and signing aren't compatible... however, 
signing and compression should be just fine (sign before compression).


-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread Kees Cook

On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
 wrote:
> On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook  wrote:
>> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>>  wrote:
>>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell  
>>> wrote:
 "Michael Kerrisk (man-pages)"  writes:
>> FIX: add flags arg to sys_finit_module()
>>
>> Thanks to Michael Kerrisk for keeping us honest.
>
> w00t! Thanks, Rusty ;-).
>
> Acked-by: Michael Kerrisk 

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell 
>>
>> Acked-by: Kees Cook 
>>
>>> I wonder if we shouldn't get a new init_module2() as well, adding the
>>> flags parameter. Of course this would be in another patch.
>>>
>>> My worries are that for compressed modules we still need to use
>>> init_module() and then --force won't work with signed modules.
>>
>> For those cases, I think it should remain up to userspace to do the
>> decompress and use init_module(). The code I'd written for patching
>> module-init-tools basically just kept the fd around if it didn't need
>> to mangle the module, and it would use finit_module (written before
>> the flags argument was added):
>>
>> /* request kernel linkage */
>> -   ret = init_module(module->data, module->len, opts);
>> +   if (fd < 0)
>> +   ret = init_module(module->data, module->len, opts);
>> +   else {
>> +   ret = finit_module(fd, opts);
>> +   if (ret != 0 && errno == ENOSYS)
>> +   ret = init_module(module->data, module->len, opts);
>> +   }
>> if (ret != 0) {
>>
>> (And yes, I realize kmod is what'll actually be getting this logic.
>> This was for my testing in Chrome OS, which is still using
>> module-init-tools.)
>
> sure... but do you realize this will fail in case kernel is checking
> module signature and we passed --force to modprobe (therefore mangled
> the decompressed memory area)?

Hm, yeah, userspace mangling of a module plus signing would fail.
Seems like mangling and signing aren't compatible. Doing it in
kernel-space (as now written for finit_module) solves that, but it
means that now compression isn't possible if you need both signing and
mangling.

I'm not a user of signing, compression, or mangling, so I'm probably a
bit unimaginative here. It seems like the case for needing all three
is pretty uncommon. (e.g. if you're doing compression, you're probably
building embedded images, which means you're unlikely to need
--force.)

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread Michael Kerrisk (man-pages)

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk 
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?

So, this more or less reached the status of an FAQ, and here are my thoughts:
http://www.kernel.org/doc/man-pages/todo.html#migrate_to_kernel_source

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread Michael Kerrisk (man-pages)

On Mon, Oct 22, 2012 at 9:39 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

So, this more or less reached the status of an FAQ, and here are my thoughts:
http://www.kernel.org/doc/man-pages/todo.html#migrate_to_kernel_source

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of The Linux Programming Interface; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread Kees Cook

On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
lucas.demar...@profusion.mobi wrote:
 On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook keesc...@chromium.org wrote:
 On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
 lucas.demar...@profusion.mobi wrote:
 On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell ru...@rustcorp.com.au 
 wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

 Acked-by: Kees Cook keesc...@chromium.org

 I wonder if we shouldn't get a new init_module2() as well, adding the
 flags parameter. Of course this would be in another patch.

 My worries are that for compressed modules we still need to use
 init_module() and then --force won't work with signed modules.

 For those cases, I think it should remain up to userspace to do the
 decompress and use init_module(). The code I'd written for patching
 module-init-tools basically just kept the fd around if it didn't need
 to mangle the module, and it would use finit_module (written before
 the flags argument was added):

 /* request kernel linkage */
 -   ret = init_module(module-data, module-len, opts);
 +   if (fd  0)
 +   ret = init_module(module-data, module-len, opts);
 +   else {
 +   ret = finit_module(fd, opts);
 +   if (ret != 0  errno == ENOSYS)
 +   ret = init_module(module-data, module-len, opts);
 +   }
 if (ret != 0) {

 (And yes, I realize kmod is what'll actually be getting this logic.
 This was for my testing in Chrome OS, which is still using
 module-init-tools.)

 sure... but do you realize this will fail in case kernel is checking
 module signature and we passed --force to modprobe (therefore mangled
 the decompressed memory area)?

Hm, yeah, userspace mangling of a module plus signing would fail.
Seems like mangling and signing aren't compatible. Doing it in
kernel-space (as now written for finit_module) solves that, but it
means that now compression isn't possible if you need both signing and
mangling.

I'm not a user of signing, compression, or mangling, so I'm probably a
bit unimaginative here. It seems like the case for needing all three
is pretty uncommon. (e.g. if you're doing compression, you're probably
building embedded images, which means you're unlikely to need
--force.)

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread H. Peter Anvin


On 10/23/2012 08:42 AM, Kees Cook wrote:


Hm, yeah, userspace mangling of a module plus signing would fail.
Seems like mangling and signing aren't compatible. Doing it in
kernel-space (as now written for finit_module) solves that, but it
means that now compression isn't possible if you need both signing and
mangling.

I'm not a user of signing, compression, or mangling, so I'm probably a
bit unimaginative here. It seems like the case for needing all three
is pretty uncommon. (e.g. if you're doing compression, you're probably
building embedded images, which means you're unlikely to need
--force.)



In particular, mangling and signing aren't compatible... however, 
signing and compression should be just fine (sign before compression).


-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-23 Thread Lucas De Marchi

On Tue, Oct 23, 2012 at 1:42 PM, Kees Cook keesc...@chromium.org wrote:
 On Mon, Oct 22, 2012 at 9:08 PM, Lucas De Marchi
 lucas.demar...@profusion.mobi wrote:
 On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook keesc...@chromium.org wrote:
 On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
 lucas.demar...@profusion.mobi wrote:
 On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell ru...@rustcorp.com.au 
 wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

 Acked-by: Kees Cook keesc...@chromium.org

 I wonder if we shouldn't get a new init_module2() as well, adding the
 flags parameter. Of course this would be in another patch.

 My worries are that for compressed modules we still need to use
 init_module() and then --force won't work with signed modules.

 For those cases, I think it should remain up to userspace to do the
 decompress and use init_module(). The code I'd written for patching
 module-init-tools basically just kept the fd around if it didn't need
 to mangle the module, and it would use finit_module (written before
 the flags argument was added):

 /* request kernel linkage */
 -   ret = init_module(module-data, module-len, opts);
 +   if (fd  0)
 +   ret = init_module(module-data, module-len, opts);
 +   else {
 +   ret = finit_module(fd, opts);
 +   if (ret != 0  errno == ENOSYS)
 +   ret = init_module(module-data, module-len, opts);
 +   }
 if (ret != 0) {

 (And yes, I realize kmod is what'll actually be getting this logic.
 This was for my testing in Chrome OS, which is still using
 module-init-tools.)

 sure... but do you realize this will fail in case kernel is checking
 module signature and we passed --force to modprobe (therefore mangled
 the decompressed memory area)?

 Hm, yeah, userspace mangling of a module plus signing would fail.
 Seems like mangling and signing aren't compatible. Doing it in
 kernel-space (as now written for finit_module) solves that, but it
 means that now compression isn't possible if you need both signing and
 mangling.

 I'm not a user of signing, compression, or mangling, so I'm probably a
 bit unimaginative here. It seems like the case for needing all three
 is pretty uncommon. (e.g. if you're doing compression, you're probably
 building embedded images, which means you're unlikely to need
 --force.)


Some desktop distros ship compressed modules by default. I received
feedback from distros some months ago that this is basically because
of the disk space, not performance.  However some measurements I did
in a regular laptop with spinning disk showed a small advantage
performance-wise, too  (with SSD is another story and uncompressed
wins by a large margin)

Since this only affects users of --force option, I think it only
affects module developers, who could uncompress the module, call
depmod again, and modprobe it. ( Mixing compressed and uncompressed
modules used not to work in module-init-tools and earlier versions of
kmod wrt dependencies, but now it should be a seamless operation )


Lucas De Marchi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook  wrote:
> On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
>  wrote:
>> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell  wrote:
>>> "Michael Kerrisk (man-pages)"  writes:
> FIX: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk 
>>>
>>> Here's the version I ended up with when I added two flags.
>>>
>>> Lucas, is this useful to you?
>>>
>>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>>
>>> Thanks,
>>> Rusty.
>>>
>>> module: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
>>> useful for eliminating the only case where kmod has to mangle a module's
>>> internals: for overriding module versioning.
>>>
>>> Signed-off-by: Rusty Russell 
>
> Acked-by: Kees Cook 
>
>> I wonder if we shouldn't get a new init_module2() as well, adding the
>> flags parameter. Of course this would be in another patch.
>>
>> My worries are that for compressed modules we still need to use
>> init_module() and then --force won't work with signed modules.
>
> For those cases, I think it should remain up to userspace to do the
> decompress and use init_module(). The code I'd written for patching
> module-init-tools basically just kept the fd around if it didn't need
> to mangle the module, and it would use finit_module (written before
> the flags argument was added):
>
> /* request kernel linkage */
> -   ret = init_module(module->data, module->len, opts);
> +   if (fd < 0)
> +   ret = init_module(module->data, module->len, opts);
> +   else {
> +   ret = finit_module(fd, opts);
> +   if (ret != 0 && errno == ENOSYS)
> +   ret = init_module(module->data, module->len, opts);
> +   }
> if (ret != 0) {
>
> (And yes, I realize kmod is what'll actually be getting this logic.
> This was for my testing in Chrome OS, which is still using
> module-init-tools.)

sure... but do you realize this will fail in case kernel is checking
module signature and we passed --force to modprobe (therefore mangled
the decompressed memory area)?


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-22 Thread Kees Cook

On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
 wrote:
> On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell  wrote:
>> "Michael Kerrisk (man-pages)"  writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.
>>>
>>> w00t! Thanks, Rusty ;-).
>>>
>>> Acked-by: Michael Kerrisk 
>>
>> Here's the version I ended up with when I added two flags.
>>
>> Lucas, is this useful to you?
>>
>> BTW Michael: why aren't the syscall man pages in the kernel source?
>>
>> Thanks,
>> Rusty.
>>
>> module: add flags arg to sys_finit_module()
>>
>> Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
>> useful for eliminating the only case where kmod has to mangle a module's
>> internals: for overriding module versioning.
>>
>> Signed-off-by: Rusty Russell 

Acked-by: Kees Cook 

> I wonder if we shouldn't get a new init_module2() as well, adding the
> flags parameter. Of course this would be in another patch.
>
> My worries are that for compressed modules we still need to use
> init_module() and then --force won't work with signed modules.

For those cases, I think it should remain up to userspace to do the
decompress and use init_module(). The code I'd written for patching
module-init-tools basically just kept the fd around if it didn't need
to mangle the module, and it would use finit_module (written before
the flags argument was added):

/* request kernel linkage */
-   ret = init_module(module->data, module->len, opts);
+   if (fd < 0)
+   ret = init_module(module->data, module->len, opts);
+   else {
+   ret = finit_module(fd, opts);
+   if (ret != 0 && errno == ENOSYS)
+   ret = init_module(module->data, module->len, opts);
+   }
if (ret != 0) {

(And yes, I realize kmod is what'll actually be getting this logic.
This was for my testing in Chrome OS, which is still using
module-init-tools.)

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>>> FIX: add flags arg to sys_finit_module()
>>>
>>> Thanks to Michael Kerrisk for keeping us honest.
>>
>> w00t! Thanks, Rusty ;-).
>>
>> Acked-by: Michael Kerrisk 
>
> Here's the version I ended up with when I added two flags.
>
> Lucas, is this useful to you?
>
> BTW Michael: why aren't the syscall man pages in the kernel source?
>
> Thanks,
> Rusty.
>
> module: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
> useful for eliminating the only case where kmod has to mangle a module's
> internals: for overriding module versioning.
>
> Signed-off-by: Rusty Russell 
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 32bc035..8cf7b50 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
>
>  asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
>  unsigned long idx1, unsigned long idx2);
> -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
> +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int 
> flags);
>  #endif
> diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
> new file mode 100644
> index 000..38da425
> --- /dev/null
> +++ b/include/uapi/linux/module.h
> @@ -0,0 +1,8 @@
> +#ifndef _UAPI_LINUX_MODULE_H
> +#define _UAPI_LINUX_MODULE_H
> +
> +/* Flags for sys_finit_module: */
> +#define MODULE_INIT_IGNORE_MODVERSIONS 1
> +#define MODULE_INIT_IGNORE_VERMAGIC2
> +
> +#endif /* _UAPI_LINUX_MODULE_H */
> diff --git a/kernel/module.c b/kernel/module.c
> index 261bf82..55b49cd 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -61,6 +61,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "module-internal.h"
>
>  #define CREATE_TRACE_POINTS
> @@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
> vfree(info->hdr);
>  }
>
> -static int rewrite_section_headers(struct load_info *info)
> +static int rewrite_section_headers(struct load_info *info, int flags)
>  {
> unsigned int i;
>
> @@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info 
> *info)
> }
>
> /* Track but don't keep modinfo and version sections. */
> -   info->index.vers = find_sec(info, "__versions");
> +   if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
> +   info->index.vers = 0; /* Pretend no __versions section! */
> +   else
> +   info->index.vers = find_sec(info, "__versions");
> info->index.info = find_sec(info, ".modinfo");
> info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
> info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
> @@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info 
> *info)
>   * Return the temporary module pointer (we'll replace it with the final
>   * one when we move the module sections around).
>   */
> -static struct module *setup_load_info(struct load_info *info)
> +static struct module *setup_load_info(struct load_info *info, int flags)
>  {
> unsigned int i;
> int err;
> @@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info 
> *info)
> info->secstrings = (void *)info->hdr
> + info->sechdrs[info->hdr->e_shstrndx].sh_offset;
>
> -   err = rewrite_section_headers(info);
> +   err = rewrite_section_headers(info, flags);
> if (err)
> return ERR_PTR(err);
>
> @@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct 
> load_info *info)
> return mod;
>  }
>
> -static int check_modinfo(struct module *mod, struct load_info *info)
> +static int check_modinfo(struct module *mod, struct load_info *info, int 
> flags)
>  {
> const char *modmagic = get_modinfo(info, "vermagic");
> int err;
>
> +   if (flags & MODULE_INIT_IGNORE_VERMAGIC)
> +   modmagic = NULL;
> +
> /* This is allowed: modprobe --force will invalidate it. */
> if (!modmagic) {
> err = try_to_force_load(mod, "bad vermagic");
> @@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
> return 0;
>  }
>
> -static struct module *layout_and_allocate(struct load_info *info)
> +static struct module *layout_and_allocate(struct load_info *info, int flags)
>  {
> /* Module within temporary copy. */
> struct module *mod;
> Elf_Shdr *pcpusec;
> int err;
>
> -   mod = setup_load_info(info);
> +   mod = setup_load_info(info, flags);
> if (IS_ERR(mod))
> return mod;
>
> -   err = check_modinfo(mod, info);
> +   err = check_modinfo(mod, info, flags);
> if (err)
> return ERR_PTR(err);
>
> @@ -3094,7 +3102,8 @@

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-22 Thread Rusty Russell

"Michael Kerrisk (man-pages)"  writes:
>> FIX: add flags arg to sys_finit_module()
>>
>> Thanks to Michael Kerrisk for keeping us honest.
>
> w00t! Thanks, Rusty ;-).
>
> Acked-by: Michael Kerrisk 

Here's the version I ended up with when I added two flags.

Lucas, is this useful to you?

BTW Michael: why aren't the syscall man pages in the kernel source?

Thanks,
Rusty.

module: add flags arg to sys_finit_module()

Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
useful for eliminating the only case where kmod has to mangle a module's
internals: for overriding module versioning.

Signed-off-by: Rusty Russell 

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 32bc035..8cf7b50 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
-asmlinkage long sys_finit_module(int fd, const char __user *uargs);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
 #endif
diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
new file mode 100644
index 000..38da425
--- /dev/null
+++ b/include/uapi/linux/module.h
@@ -0,0 +1,8 @@
+#ifndef _UAPI_LINUX_MODULE_H
+#define _UAPI_LINUX_MODULE_H
+
+/* Flags for sys_finit_module: */
+#define MODULE_INIT_IGNORE_MODVERSIONS 1
+#define MODULE_INIT_IGNORE_VERMAGIC2
+
+#endif /* _UAPI_LINUX_MODULE_H */
diff --git a/kernel/module.c b/kernel/module.c
index 261bf82..55b49cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -61,6 +61,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "module-internal.h"
 
 #define CREATE_TRACE_POINTS
@@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
vfree(info->hdr);
 }
 
-static int rewrite_section_headers(struct load_info *info)
+static int rewrite_section_headers(struct load_info *info, int flags)
 {
unsigned int i;
 
@@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info 
*info)
}
 
/* Track but don't keep modinfo and version sections. */
-   info->index.vers = find_sec(info, "__versions");
+   if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
+   info->index.vers = 0; /* Pretend no __versions section! */
+   else
+   info->index.vers = find_sec(info, "__versions");
info->index.info = find_sec(info, ".modinfo");
info->sechdrs[info->index.info].sh_flags &= ~(unsigned long)SHF_ALLOC;
info->sechdrs[info->index.vers].sh_flags &= ~(unsigned long)SHF_ALLOC;
@@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info *info)
  * Return the temporary module pointer (we'll replace it with the final
  * one when we move the module sections around).
  */
-static struct module *setup_load_info(struct load_info *info)
+static struct module *setup_load_info(struct load_info *info, int flags)
 {
unsigned int i;
int err;
@@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info 
*info)
info->secstrings = (void *)info->hdr
+ info->sechdrs[info->hdr->e_shstrndx].sh_offset;
 
-   err = rewrite_section_headers(info);
+   err = rewrite_section_headers(info, flags);
if (err)
return ERR_PTR(err);
 
@@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct load_info 
*info)
return mod;
 }
 
-static int check_modinfo(struct module *mod, struct load_info *info)
+static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 {
const char *modmagic = get_modinfo(info, "vermagic");
int err;
 
+   if (flags & MODULE_INIT_IGNORE_VERMAGIC)
+   modmagic = NULL;
+
/* This is allowed: modprobe --force will invalidate it. */
if (!modmagic) {
err = try_to_force_load(mod, "bad vermagic");
@@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
return 0;
 }
 
-static struct module *layout_and_allocate(struct load_info *info)
+static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
/* Module within temporary copy. */
struct module *mod;
Elf_Shdr *pcpusec;
int err;
 
-   mod = setup_load_info(info);
+   mod = setup_load_info(info, flags);
if (IS_ERR(mod))
return mod;
 
-   err = check_modinfo(mod, info);
+   err = check_modinfo(mod, info, flags);
if (err)
return ERR_PTR(err);
 
@@ -3094,7 +3102,8 @@ static int may_init_module(void)
 
 /* Allocate and load the module: note that size of section 0 is always
zero, and we rely on this for optional sections. */
-static int load_module(struct load_info *info, const char __user *uargs)
+static int load_module(struct load_info *info, const char __user *uargs,
+

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-22 Thread Rusty Russell

Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

Here's the version I ended up with when I added two flags.

Lucas, is this useful to you?

BTW Michael: why aren't the syscall man pages in the kernel source?

Thanks,
Rusty.

module: add flags arg to sys_finit_module()

Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
useful for eliminating the only case where kmod has to mangle a module's
internals: for overriding module versioning.

Signed-off-by: Rusty Russell ru...@rustcorp.com.au

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 32bc035..8cf7b50 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
-asmlinkage long sys_finit_module(int fd, const char __user *uargs);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
 #endif
diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
new file mode 100644
index 000..38da425
--- /dev/null
+++ b/include/uapi/linux/module.h
@@ -0,0 +1,8 @@
+#ifndef _UAPI_LINUX_MODULE_H
+#define _UAPI_LINUX_MODULE_H
+
+/* Flags for sys_finit_module: */
+#define MODULE_INIT_IGNORE_MODVERSIONS 1
+#define MODULE_INIT_IGNORE_VERMAGIC2
+
+#endif /* _UAPI_LINUX_MODULE_H */
diff --git a/kernel/module.c b/kernel/module.c
index 261bf82..55b49cd 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -61,6 +61,7 @@
 #include linux/pfn.h
 #include linux/bsearch.h
 #include linux/fips.h
+#include uapi/linux/module.h
 #include module-internal.h
 
 #define CREATE_TRACE_POINTS
@@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
vfree(info-hdr);
 }
 
-static int rewrite_section_headers(struct load_info *info)
+static int rewrite_section_headers(struct load_info *info, int flags)
 {
unsigned int i;
 
@@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info 
*info)
}
 
/* Track but don't keep modinfo and version sections. */
-   info-index.vers = find_sec(info, __versions);
+   if (flags  MODULE_INIT_IGNORE_MODVERSIONS)
+   info-index.vers = 0; /* Pretend no __versions section! */
+   else
+   info-index.vers = find_sec(info, __versions);
info-index.info = find_sec(info, .modinfo);
info-sechdrs[info-index.info].sh_flags = ~(unsigned long)SHF_ALLOC;
info-sechdrs[info-index.vers].sh_flags = ~(unsigned long)SHF_ALLOC;
@@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info *info)
  * Return the temporary module pointer (we'll replace it with the final
  * one when we move the module sections around).
  */
-static struct module *setup_load_info(struct load_info *info)
+static struct module *setup_load_info(struct load_info *info, int flags)
 {
unsigned int i;
int err;
@@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info 
*info)
info-secstrings = (void *)info-hdr
+ info-sechdrs[info-hdr-e_shstrndx].sh_offset;
 
-   err = rewrite_section_headers(info);
+   err = rewrite_section_headers(info, flags);
if (err)
return ERR_PTR(err);
 
@@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct load_info 
*info)
return mod;
 }
 
-static int check_modinfo(struct module *mod, struct load_info *info)
+static int check_modinfo(struct module *mod, struct load_info *info, int flags)
 {
const char *modmagic = get_modinfo(info, vermagic);
int err;
 
+   if (flags  MODULE_INIT_IGNORE_VERMAGIC)
+   modmagic = NULL;
+
/* This is allowed: modprobe --force will invalidate it. */
if (!modmagic) {
err = try_to_force_load(mod, bad vermagic);
@@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
return 0;
 }
 
-static struct module *layout_and_allocate(struct load_info *info)
+static struct module *layout_and_allocate(struct load_info *info, int flags)
 {
/* Module within temporary copy. */
struct module *mod;
Elf_Shdr *pcpusec;
int err;
 
-   mod = setup_load_info(info);
+   mod = setup_load_info(info, flags);
if (IS_ERR(mod))
return mod;
 
-   err = check_modinfo(mod, info);
+   err = check_modinfo(mod, info, flags);
if (err)
return ERR_PTR(err);
 
@@ -3094,7 +3102,8 @@ static int may_init_module(void)
 
 /* Allocate and load the module: note that size of section 0 is always
zero, and we rely on this for optional sections. */
-static int load_module(struct load_info *info, const char __user

Re: [PATCH 1/4] module: add syscall to load module from fd

On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

 diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
 index 32bc035..8cf7b50 100644
 --- a/include/linux/syscalls.h
 +++ b/include/linux/syscalls.h
 @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,

  asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
  unsigned long idx1, unsigned long idx2);
 -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int 
 flags);
  #endif
 diff --git a/include/uapi/linux/module.h b/include/uapi/linux/module.h
 new file mode 100644
 index 000..38da425
 --- /dev/null
 +++ b/include/uapi/linux/module.h
 @@ -0,0 +1,8 @@
 +#ifndef _UAPI_LINUX_MODULE_H
 +#define _UAPI_LINUX_MODULE_H
 +
 +/* Flags for sys_finit_module: */
 +#define MODULE_INIT_IGNORE_MODVERSIONS 1
 +#define MODULE_INIT_IGNORE_VERMAGIC2
 +
 +#endif /* _UAPI_LINUX_MODULE_H */
 diff --git a/kernel/module.c b/kernel/module.c
 index 261bf82..55b49cd 100644
 --- a/kernel/module.c
 +++ b/kernel/module.c
 @@ -61,6 +61,7 @@
  #include linux/pfn.h
  #include linux/bsearch.h
  #include linux/fips.h
 +#include uapi/linux/module.h
  #include module-internal.h

  #define CREATE_TRACE_POINTS
 @@ -2569,7 +2570,7 @@ static void free_copy(struct load_info *info)
 vfree(info-hdr);
  }

 -static int rewrite_section_headers(struct load_info *info)
 +static int rewrite_section_headers(struct load_info *info, int flags)
  {
 unsigned int i;

 @@ -2597,7 +2598,10 @@ static int rewrite_section_headers(struct load_info 
 *info)
 }

 /* Track but don't keep modinfo and version sections. */
 -   info-index.vers = find_sec(info, __versions);
 +   if (flags  MODULE_INIT_IGNORE_MODVERSIONS)
 +   info-index.vers = 0; /* Pretend no __versions section! */
 +   else
 +   info-index.vers = find_sec(info, __versions);
 info-index.info = find_sec(info, .modinfo);
 info-sechdrs[info-index.info].sh_flags = ~(unsigned long)SHF_ALLOC;
 info-sechdrs[info-index.vers].sh_flags = ~(unsigned long)SHF_ALLOC;
 @@ -2612,7 +2617,7 @@ static int rewrite_section_headers(struct load_info 
 *info)
   * Return the temporary module pointer (we'll replace it with the final
   * one when we move the module sections around).
   */
 -static struct module *setup_load_info(struct load_info *info)
 +static struct module *setup_load_info(struct load_info *info, int flags)
  {
 unsigned int i;
 int err;
 @@ -2623,7 +2628,7 @@ static struct module *setup_load_info(struct load_info 
 *info)
 info-secstrings = (void *)info-hdr
 + info-sechdrs[info-hdr-e_shstrndx].sh_offset;

 -   err = rewrite_section_headers(info);
 +   err = rewrite_section_headers(info, flags);
 if (err)
 return ERR_PTR(err);

 @@ -2661,11 +2666,14 @@ static struct module *setup_load_info(struct 
 load_info *info)
 return mod;
  }

 -static int check_modinfo(struct module *mod, struct load_info *info)
 +static int check_modinfo(struct module *mod, struct load_info *info, int 
 flags)
  {
 const char *modmagic = get_modinfo(info, vermagic);
 int err;

 +   if (flags  MODULE_INIT_IGNORE_VERMAGIC)
 +   modmagic = NULL;
 +
 /* This is allowed: modprobe --force will invalidate it. */
 if (!modmagic) {
 err = try_to_force_load(mod, bad vermagic);
 @@ -2901,18 +2909,18 @@ int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
 return 0;
  }

 -static struct module *layout_and_allocate(struct load_info *info)
 +static struct module *layout_and_allocate(struct load_info *info, int flags)
  {
 /* Module within temporary copy. */
 struct module *mod;
 Elf_Shdr *pcpusec;
 int err;

 -   mod = setup_load_info(info);
 +   mod = setup_load_info(info, flags);
 if (IS_ERR(mod))
 return mod;

 -   err = check_modinfo(mod, info);
 +   err = check_modinfo(mod, info, flags);
 if (err)
 return ERR_PTR(err);

 @@ -3094,7 +3102,8 @@ static int may_init_module(void)

  /* Allocate

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-22 Thread Kees Cook

On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
lucas.demar...@profusion.mobi wrote:
 On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

Acked-by: Kees Cook keesc...@chromium.org

 I wonder if we shouldn't get a new init_module2() as well, adding the
 flags parameter. Of course this would be in another patch.

 My worries are that for compressed modules we still need to use
 init_module() and then --force won't work with signed modules.

For those cases, I think it should remain up to userspace to do the
decompress and use init_module(). The code I'd written for patching
module-init-tools basically just kept the fd around if it didn't need
to mangle the module, and it would use finit_module (written before
the flags argument was added):

/* request kernel linkage */
-   ret = init_module(module-data, module-len, opts);
+   if (fd  0)
+   ret = init_module(module-data, module-len, opts);
+   else {
+   ret = finit_module(fd, opts);
+   if (ret != 0  errno == ENOSYS)
+   ret = init_module(module-data, module-len, opts);
+   }
if (ret != 0) {

(And yes, I realize kmod is what'll actually be getting this logic.
This was for my testing in Chrome OS, which is still using
module-init-tools.)

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 23, 2012 at 1:40 AM, Kees Cook keesc...@chromium.org wrote:
 On Mon, Oct 22, 2012 at 7:37 PM, Lucas De Marchi
 lucas.demar...@profusion.mobi wrote:
 On Mon, Oct 22, 2012 at 5:39 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 w00t! Thanks, Rusty ;-).

 Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 Here's the version I ended up with when I added two flags.

 Lucas, is this useful to you?

 BTW Michael: why aren't the syscall man pages in the kernel source?

 Thanks,
 Rusty.

 module: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.  These flags are actually
 useful for eliminating the only case where kmod has to mangle a module's
 internals: for overriding module versioning.

 Signed-off-by: Rusty Russell ru...@rustcorp.com.au

 Acked-by: Kees Cook keesc...@chromium.org

 I wonder if we shouldn't get a new init_module2() as well, adding the
 flags parameter. Of course this would be in another patch.

 My worries are that for compressed modules we still need to use
 init_module() and then --force won't work with signed modules.

 For those cases, I think it should remain up to userspace to do the
 decompress and use init_module(). The code I'd written for patching
 module-init-tools basically just kept the fd around if it didn't need
 to mangle the module, and it would use finit_module (written before
 the flags argument was added):

 /* request kernel linkage */
 -   ret = init_module(module-data, module-len, opts);
 +   if (fd  0)
 +   ret = init_module(module-data, module-len, opts);
 +   else {
 +   ret = finit_module(fd, opts);
 +   if (ret != 0  errno == ENOSYS)
 +   ret = init_module(module-data, module-len, opts);
 +   }
 if (ret != 0) {

 (And yes, I realize kmod is what'll actually be getting this logic.
 This was for my testing in Chrome OS, which is still using
 module-init-tools.)

sure... but do you realize this will fail in case kernel is checking
module signature and we passed --force to modprobe (therefore mangled
the decompressed memory area)?


Lucas De Marchi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-19 Thread Rusty Russell

"H. Peter Anvin"  writes:
> On 10/18/2012 07:23 PM, Rusty Russell wrote:
>> "H. Peter Anvin"  writes:
>>> Given that, I have to say I now seriously question the value of
>>> finit_module().  The kernel can trivially discover if the pointed-to
>>> memory area is a MAP_SHARED mmap() of a file descriptor and if so which
>>> file descriptor... why can't we handle this behind the scenes?
>>
>> It is a bit more indirect, but also in practice it's a bit trickier than
>> that.  We need to ensure the memory doesn't change underneath us and
>> stays attached to that fd.  I can easily see that code slipping and
>> ending in an exploit.
>>
>> But that may be my irrational fear of the mm :)
>
> You have to do the same thing with a file/file descriptor, I would think.

After the fget(fd), it can't change where it's attached to though.
Ensuring that the fd behind the shared region is trusted and doesn't
change between the check and the read has more atomicity issues.

> However, I keep wondering about the use case for this, as opposed to 
> signatures.

The IMA people are signing every file in xattrs; this makes it trivial
for them to extend the same mechanism to kernel modules (though they'll
probably want to add xattrs to our cpio support, but bsdcpio et al
already have cpio-with-xattrs so I doubt it'll be hard).

And Kees simply has a known-secure partition, IIUC, which makes their
verification step pretty trivial.

The opportunity to add a flags arg is just the icing on the cake, but I
think the combination is compelling.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-19 Thread Alon Ziv

H. Peter Anvin  zytor.com> writes:
> > It is a bit more indirect, but also in practice it's a bit trickier than
> > that.  We need to ensure the memory doesn't change underneath us and
> > stays attached to that fd.  I can easily see that code slipping and
> > ending in an exploit.
> >
> > But that may be my irrational fear of the mm :)
> 
> You have to do the same thing with a file/file descriptor, I would think.
> 
> However, I keep wondering about the use case for this, as opposed to 
> signatures.

Two things:
1. finit_module() lets LSMs make decisions based on full information on the
   module to be loaded
2. On some systems (such as Chromium OS) we have a trusted root OS (e.g. the
   entire root filesystem is protected using dm-verity); requiring signatures
   on top of this is a waste of resources


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-19 Thread Alon Ziv

H. Peter Anvin hpa at zytor.com writes:
  It is a bit more indirect, but also in practice it's a bit trickier than
  that.  We need to ensure the memory doesn't change underneath us and
  stays attached to that fd.  I can easily see that code slipping and
  ending in an exploit.
 
  But that may be my irrational fear of the mm :)
 
 You have to do the same thing with a file/file descriptor, I would think.
 
 However, I keep wondering about the use case for this, as opposed to 
 signatures.

Two things:
1. finit_module() lets LSMs make decisions based on full information on the
   module to be loaded
2. On some systems (such as Chromium OS) we have a trusted root OS (e.g. the
   entire root filesystem is protected using dm-verity); requiring signatures
   on top of this is a waste of resources


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-19 Thread Rusty Russell

H. Peter Anvin h...@zytor.com writes:
 On 10/18/2012 07:23 PM, Rusty Russell wrote:
 H. Peter Anvin h...@zytor.com writes:
 Given that, I have to say I now seriously question the value of
 finit_module().  The kernel can trivially discover if the pointed-to
 memory area is a MAP_SHARED mmap() of a file descriptor and if so which
 file descriptor... why can't we handle this behind the scenes?

 It is a bit more indirect, but also in practice it's a bit trickier than
 that.  We need to ensure the memory doesn't change underneath us and
 stays attached to that fd.  I can easily see that code slipping and
 ending in an exploit.

 But that may be my irrational fear of the mm :)

 You have to do the same thing with a file/file descriptor, I would think.

After the fget(fd), it can't change where it's attached to though.
Ensuring that the fd behind the shared region is trusted and doesn't
change between the check and the read has more atomicity issues.

 However, I keep wondering about the use case for this, as opposed to 
 signatures.

The IMA people are signing every file in xattrs; this makes it trivial
for them to extend the same mechanism to kernel modules (though they'll
probably want to add xattrs to our cpio support, but bsdcpio et al
already have cpio-with-xattrs so I doubt it'll be hard).

And Kees simply has a known-secure partition, IIUC, which makes their
verification step pretty trivial.

The opportunity to add a flags arg is just the icing on the cake, but I
think the combination is compelling.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd


On 10/18/2012 07:23 PM, Rusty Russell wrote:

"H. Peter Anvin"  writes:

Given that, I have to say I now seriously question the value of
finit_module().  The kernel can trivially discover if the pointed-to
memory area is a MAP_SHARED mmap() of a file descriptor and if so which
file descriptor... why can't we handle this behind the scenes?


It is a bit more indirect, but also in practice it's a bit trickier than
that.  We need to ensure the memory doesn't change underneath us and
stays attached to that fd.  I can easily see that code slipping and
ending in an exploit.

But that may be my irrational fear of the mm :)


You have to do the same thing with a file/file descriptor, I would think.

However, I keep wondering about the use case for this, as opposed to 
signatures.


-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-18 Thread Rusty Russell

"H. Peter Anvin"  writes:
> Given that, I have to say I now seriously question the value of 
> finit_module().  The kernel can trivially discover if the pointed-to 
> memory area is a MAP_SHARED mmap() of a file descriptor and if so which 
> file descriptor... why can't we handle this behind the scenes?

It is a bit more indirect, but also in practice it's a bit trickier than
that.  We need to ensure the memory doesn't change underneath us and
stays attached to that fd.  I can easily see that code slipping and
ending in an exploit.

But that may be my irrational fear of the mm :)

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd


On 10/18/2012 08:28 AM, Kees Cook wrote:


The goal for finit_module is to make sure we're getting what's on the
filesystem, not an arbitrary blob, so we can reason about it for
security policy.



Yes, I get that... although I'm starting to think that that might 
actually be a really bad idea.



was confused about the functioning of the *current* init_module() system
call.

Given that, I have to say I now seriously question the value of
finit_module().  The kernel can trivially discover if the pointed-to memory
area is a MAP_SHARED mmap() of a file descriptor and if so which file
descriptor... why can't we handle this behind the scenes?


This makes me very nervous. I worry that it adds needless complexity
(it'd be many more checks besides "is it MAP_SHARED?", like "does the
memory region show the whole file?" "is the offset zero?" etc). Also
are we sure the memory area would be truly be unmodifiable in the case
where the filesystem is read-only?


You may need to check for PROT_READONLY as well.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-18 Thread Kees Cook

On Thu, Oct 18, 2012 at 7:26 AM, H. Peter Anvin  wrote:
> On 10/18/2012 01:05 AM, Michael Kerrisk (man-pages) wrote:
>>>
>>>
>>> So perhaps what we *should* have is something that points to the module
>>> to a (buffer, length) in userspace, and the equivalent of the current
>>> init_module() would be open() + mmap() + minit_module() + close()?
>>
>>
>> So, I don't get it. What are the args you propose for of minit_module()?
>>
>
> Nevermind, this is what the current init_module() already takes.
>
> So it sounds like Rusty is objecting to the very notion of tying a module to
> a file descriptor the way the proposed finit_module() system call does -- I

The goal for finit_module is to make sure we're getting what's on the
filesystem, not an arbitrary blob, so we can reason about it for
security policy.

> was confused about the functioning of the *current* init_module() system
> call.
>
> Given that, I have to say I now seriously question the value of
> finit_module().  The kernel can trivially discover if the pointed-to memory
> area is a MAP_SHARED mmap() of a file descriptor and if so which file
> descriptor... why can't we handle this behind the scenes?

This makes me very nervous. I worry that it adds needless complexity
(it'd be many more checks besides "is it MAP_SHARED?", like "does the
memory region show the whole file?" "is the offset zero?" etc). Also
are we sure the memory area would be truly be unmodifiable in the case
where the filesystem is read-only?

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd


On 10/18/2012 01:05 AM, Michael Kerrisk (man-pages) wrote:


So perhaps what we *should* have is something that points to the module
to a (buffer, length) in userspace, and the equivalent of the current
init_module() would be open() + mmap() + minit_module() + close()?


So, I don't get it. What are the args you propose for of minit_module()?



Nevermind, this is what the current init_module() already takes.

So it sounds like Rusty is objecting to the very notion of tying a 
module to a file descriptor the way the proposed finit_module() system 
call does -- I was confused about the functioning of the *current* 
init_module() system call.


Given that, I have to say I now seriously question the value of 
finit_module().  The kernel can trivially discover if the pointed-to 
memory area is a MAP_SHARED mmap() of a file descriptor and if so which 
file descriptor... why can't we handle this behind the scenes?


-hpa

P.S. the man page for init_module(2) is seriously out of date...

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 5:12 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>> Sure. But my point that started this subthread was: should we take the
>> opportunity now to add a 'flags' argument to the new finit_module()
>> system call, so as to allow flexibility in extending the behavior in
>> future? There have been so many cases of revised system calls in the
>> past few years that replaced calls without a 'flags' argument that it
>> seems worth at least some thought before the API is cast in stone.
>
> (CC's trimmed, Lucas & Jon added; please include them in module
> discussions!)
>
> So I tried to think of why we'd want flags; if I could think of a
> plausible reason, obviously we should do it now.
>
> I think it would be neat for the force flags (eg. ignoring modversions
> or ignoring kernel version).  These are the only cases where libkmod
> needs to mangle the module.
>
> So here's the patch which adds the flags field, but nothing in there
> yet.  I'll add the remove flags soon, so libkmod can assume that if the
> syscall exists, those flags will work.
>
> Thoughts?
> Rusty.
>
> FIX: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.

w00t! Thanks, Rusty ;-).

Acked-by: Michael Kerrisk 

> +   if (flags)
> +   return -EINVAL;

And thanks for that check. So easy, so obvious, and so often forgotten.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 6:24 AM, H. Peter Anvin  wrote:
> On 10/11/2012 03:16 PM, Rusty Russell wrote:
>> "H. Peter Anvin"  writes:
>>
>>> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
 Good point. A "whole hog" openat()-style interface is worth thinking about 
 too.
>>>
>>> *Although* you could argue that you can always simply open the module
>>> file first, and that finit_module() is really what we should have had in
>>> the first place.  Then you don't need the flags since those would come
>>> from openat().
>>
>> There's no fundamental reason that modules have to be in a file.  I'm
>> thinking of compressed modules, or an initrd which simply includes all
>> the modules it wants to load in one linear file.
>>
>> Also, --force options manipulate the module before loading (as did the
>> now-obsolete module rename option).
>>
>
> So perhaps what we *should* have is something that points to the module
> to a (buffer, length) in userspace, and the equivalent of the current
> init_module() would be open() + mmap() + minit_module() + close()?

So, I don't get it. What are the args you propose for of minit_module()?


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 6:24 AM, H. Peter Anvin h...@zytor.com wrote:
 On 10/11/2012 03:16 PM, Rusty Russell wrote:
 H. Peter Anvin h...@zytor.com writes:

 On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
 Good point. A whole hog openat()-style interface is worth thinking about 
 too.

 *Although* you could argue that you can always simply open the module
 file first, and that finit_module() is really what we should have had in
 the first place.  Then you don't need the flags since those would come
 from openat().

 There's no fundamental reason that modules have to be in a file.  I'm
 thinking of compressed modules, or an initrd which simply includes all
 the modules it wants to load in one linear file.

 Also, --force options manipulate the module before loading (as did the
 now-obsolete module rename option).


 So perhaps what we *should* have is something that points to the module
 to a (buffer, length) in userspace, and the equivalent of the current
 init_module() would be open() + mmap() + minit_module() + close()?

So, I don't get it. What are the args you propose for of minit_module()?


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of The Linux Programming Interface; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Oct 18, 2012 at 5:12 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 Sure. But my point that started this subthread was: should we take the
 opportunity now to add a 'flags' argument to the new finit_module()
 system call, so as to allow flexibility in extending the behavior in
 future? There have been so many cases of revised system calls in the
 past few years that replaced calls without a 'flags' argument that it
 seems worth at least some thought before the API is cast in stone.

 (CC's trimmed, Lucas  Jon added; please include them in module
 discussions!)

 So I tried to think of why we'd want flags; if I could think of a
 plausible reason, obviously we should do it now.

 I think it would be neat for the force flags (eg. ignoring modversions
 or ignoring kernel version).  These are the only cases where libkmod
 needs to mangle the module.

 So here's the patch which adds the flags field, but nothing in there
 yet.  I'll add the remove flags soon, so libkmod can assume that if the
 syscall exists, those flags will work.

 Thoughts?
 Rusty.

 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

w00t! Thanks, Rusty ;-).

Acked-by: Michael Kerrisk mtk.manpa...@gmail.com

 +   if (flags)
 +   return -EINVAL;

And thanks for that check. So easy, so obvious, and so often forgotten.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of The Linux Programming Interface; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd


On 10/18/2012 01:05 AM, Michael Kerrisk (man-pages) wrote:


So perhaps what we *should* have is something that points to the module
to a (buffer, length) in userspace, and the equivalent of the current
init_module() would be open() + mmap() + minit_module() + close()?


So, I don't get it. What are the args you propose for of minit_module()?



Nevermind, this is what the current init_module() already takes.

So it sounds like Rusty is objecting to the very notion of tying a 
module to a file descriptor the way the proposed finit_module() system 
call does -- I was confused about the functioning of the *current* 
init_module() system call.


Given that, I have to say I now seriously question the value of 
finit_module().  The kernel can trivially discover if the pointed-to 
memory area is a MAP_SHARED mmap() of a file descriptor and if so which 
file descriptor... why can't we handle this behind the scenes?


-hpa

P.S. the man page for init_module(2) is seriously out of date...

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-18 Thread Kees Cook

On Thu, Oct 18, 2012 at 7:26 AM, H. Peter Anvin h...@zytor.com wrote:
 On 10/18/2012 01:05 AM, Michael Kerrisk (man-pages) wrote:


 So perhaps what we *should* have is something that points to the module
 to a (buffer, length) in userspace, and the equivalent of the current
 init_module() would be open() + mmap() + minit_module() + close()?


 So, I don't get it. What are the args you propose for of minit_module()?


 Nevermind, this is what the current init_module() already takes.

 So it sounds like Rusty is objecting to the very notion of tying a module to
 a file descriptor the way the proposed finit_module() system call does -- I

The goal for finit_module is to make sure we're getting what's on the
filesystem, not an arbitrary blob, so we can reason about it for
security policy.

 was confused about the functioning of the *current* init_module() system
 call.

 Given that, I have to say I now seriously question the value of
 finit_module().  The kernel can trivially discover if the pointed-to memory
 area is a MAP_SHARED mmap() of a file descriptor and if so which file
 descriptor... why can't we handle this behind the scenes?

This makes me very nervous. I worry that it adds needless complexity
(it'd be many more checks besides is it MAP_SHARED?, like does the
memory region show the whole file? is the offset zero? etc). Also
are we sure the memory area would be truly be unmodifiable in the case
where the filesystem is read-only?

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd


On 10/18/2012 08:28 AM, Kees Cook wrote:


The goal for finit_module is to make sure we're getting what's on the
filesystem, not an arbitrary blob, so we can reason about it for
security policy.



Yes, I get that... although I'm starting to think that that might 
actually be a really bad idea.



was confused about the functioning of the *current* init_module() system
call.

Given that, I have to say I now seriously question the value of
finit_module().  The kernel can trivially discover if the pointed-to memory
area is a MAP_SHARED mmap() of a file descriptor and if so which file
descriptor... why can't we handle this behind the scenes?


This makes me very nervous. I worry that it adds needless complexity
(it'd be many more checks besides is it MAP_SHARED?, like does the
memory region show the whole file? is the offset zero? etc). Also
are we sure the memory area would be truly be unmodifiable in the case
where the filesystem is read-only?


You may need to check for PROT_READONLY as well.

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-18 Thread Rusty Russell

H. Peter Anvin h...@zytor.com writes:
 Given that, I have to say I now seriously question the value of 
 finit_module().  The kernel can trivially discover if the pointed-to 
 memory area is a MAP_SHARED mmap() of a file descriptor and if so which 
 file descriptor... why can't we handle this behind the scenes?

It is a bit more indirect, but also in practice it's a bit trickier than
that.  We need to ensure the memory doesn't change underneath us and
stays attached to that fd.  I can easily see that code slipping and
ending in an exploit.

But that may be my irrational fear of the mm :)

Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd


On 10/18/2012 07:23 PM, Rusty Russell wrote:

H. Peter Anvin h...@zytor.com writes:

Given that, I have to say I now seriously question the value of
finit_module().  The kernel can trivially discover if the pointed-to
memory area is a MAP_SHARED mmap() of a file descriptor and if so which
file descriptor... why can't we handle this behind the scenes?


It is a bit more indirect, but also in practice it's a bit trickier than
that.  We need to ensure the memory doesn't change underneath us and
stays attached to that fd.  I can easily see that code slipping and
ending in an exploit.

But that may be my irrational fear of the mm :)


You have to do the same thing with a file/file descriptor, I would think.

However, I keep wondering about the use case for this, as opposed to 
signatures.


-hpa


--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-17 Thread Lucas De Marchi

On Thu, Oct 18, 2012 at 12:12 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>> Sure. But my point that started this subthread was: should we take the
>> opportunity now to add a 'flags' argument to the new finit_module()
>> system call, so as to allow flexibility in extending the behavior in
>> future? There have been so many cases of revised system calls in the
>> past few years that replaced calls without a 'flags' argument that it
>> seems worth at least some thought before the API is cast in stone.
>
> (CC's trimmed, Lucas & Jon added; please include them in module
> discussions!)
>
> So I tried to think of why we'd want flags; if I could think of a
> plausible reason, obviously we should do it now.
>
> I think it would be neat for the force flags (eg. ignoring modversions
> or ignoring kernel version).  These are the only cases where libkmod
> needs to mangle the module.

Maybe we should put this back into kernel. With an fd we can't mangle
the module anymore to ignore modversions or kernel version.

So yes, I think a 'flags' param is indeed needed.

Side note:  force won't work anymore by using init_module() and signed modules.

>
> So here's the patch which adds the flags field, but nothing in there
> yet.  I'll add the remove flags soon, so libkmod can assume that if the
> syscall exists, those flags will work.
>
> Thoughts?
> Rusty.
>
> FIX: add flags arg to sys_finit_module()
>
> Thanks to Michael Kerrisk for keeping us honest.
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 32bc035..8cf7b50 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
>
>  asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
>  unsigned long idx1, unsigned long idx2);
> -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
> +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int 
> flags);
>  #endif
> diff --git a/kernel/module.c b/kernel/module.c
> index 261bf82..8b8d986 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3260,7 +3260,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
> return load_module(, uargs);
>  }
>
> -SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs, int, 
> flags)
>  {
> int err;
> struct load_info info = { };
> @@ -3269,7 +3269,11 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char 
> __user *, uargs)
> if (err)
> return err;
>
> -   pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
> +   pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, 
> flags);
> +
> +   /* Coming RSN... */
> +   if (flags)
> +   return -EINVAL;
>
> err = copy_module_from_fd(fd, );
> if (err)


Ack.

Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-17 Thread H. Peter Anvin

On 10/11/2012 03:16 PM, Rusty Russell wrote:
> "H. Peter Anvin"  writes:
> 
>> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>>> Good point. A "whole hog" openat()-style interface is worth thinking about 
>>> too.
>>
>> *Although* you could argue that you can always simply open the module
>> file first, and that finit_module() is really what we should have had in
>> the first place.  Then you don't need the flags since those would come
>> from openat().
> 
> There's no fundamental reason that modules have to be in a file.  I'm
> thinking of compressed modules, or an initrd which simply includes all
> the modules it wants to load in one linear file.
> 
> Also, --force options manipulate the module before loading (as did the
> now-obsolete module rename option).
> 

So perhaps what we *should* have is something that points to the module
to a (buffer, length) in userspace, and the equivalent of the current
init_module() would be open() + mmap() + minit_module() + close()?

-hpa



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-17 Thread Rusty Russell

"Michael Kerrisk (man-pages)"  writes:
> Sure. But my point that started this subthread was: should we take the
> opportunity now to add a 'flags' argument to the new finit_module()
> system call, so as to allow flexibility in extending the behavior in
> future? There have been so many cases of revised system calls in the
> past few years that replaced calls without a 'flags' argument that it
> seems worth at least some thought before the API is cast in stone.

(CC's trimmed, Lucas & Jon added; please include them in module
discussions!)

So I tried to think of why we'd want flags; if I could think of a
plausible reason, obviously we should do it now.

I think it would be neat for the force flags (eg. ignoring modversions
or ignoring kernel version).  These are the only cases where libkmod
needs to mangle the module.

So here's the patch which adds the flags field, but nothing in there
yet.  I'll add the remove flags soon, so libkmod can assume that if the
syscall exists, those flags will work.

Thoughts?
Rusty.

FIX: add flags arg to sys_finit_module()

Thanks to Michael Kerrisk for keeping us honest.

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 32bc035..8cf7b50 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
-asmlinkage long sys_finit_module(int fd, const char __user *uargs);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 261bf82..8b8d986 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3260,7 +3260,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
return load_module(, uargs);
 }
 
-SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
+SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs, int, flags)
 {
int err;
struct load_info info = { };
@@ -3269,7 +3269,11 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char __user 
*, uargs)
if (err)
return err;
 
-   pr_debug("finit_module: fd=%d, uargs=%p\n", fd, uargs);
+   pr_debug("finit_module: fd=%d, uargs=%p, flags=%i\n", fd, uargs, flags);
+
+   /* Coming RSN... */
+   if (flags)
+   return -EINVAL;
 
err = copy_module_from_fd(fd, );
if (err)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-17 Thread Rusty Russell

Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 Sure. But my point that started this subthread was: should we take the
 opportunity now to add a 'flags' argument to the new finit_module()
 system call, so as to allow flexibility in extending the behavior in
 future? There have been so many cases of revised system calls in the
 past few years that replaced calls without a 'flags' argument that it
 seems worth at least some thought before the API is cast in stone.

(CC's trimmed, Lucas  Jon added; please include them in module
discussions!)

So I tried to think of why we'd want flags; if I could think of a
plausible reason, obviously we should do it now.

I think it would be neat for the force flags (eg. ignoring modversions
or ignoring kernel version).  These are the only cases where libkmod
needs to mangle the module.

So here's the patch which adds the flags field, but nothing in there
yet.  I'll add the remove flags soon, so libkmod can assume that if the
syscall exists, those flags will work.

Thoughts?
Rusty.

FIX: add flags arg to sys_finit_module()

Thanks to Michael Kerrisk for keeping us honest.

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 32bc035..8cf7b50 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
-asmlinkage long sys_finit_module(int fd, const char __user *uargs);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 261bf82..8b8d986 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3260,7 +3260,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
return load_module(info, uargs);
 }
 
-SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
+SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs, int, flags)
 {
int err;
struct load_info info = { };
@@ -3269,7 +3269,11 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char __user 
*, uargs)
if (err)
return err;
 
-   pr_debug(finit_module: fd=%d, uargs=%p\n, fd, uargs);
+   pr_debug(finit_module: fd=%d, uargs=%p, flags=%i\n, fd, uargs, flags);
+
+   /* Coming RSN... */
+   if (flags)
+   return -EINVAL;
 
err = copy_module_from_fd(fd, info);
if (err)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-17 Thread H. Peter Anvin

On 10/11/2012 03:16 PM, Rusty Russell wrote:
 H. Peter Anvin h...@zytor.com writes:
 
 On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
 Good point. A whole hog openat()-style interface is worth thinking about 
 too.

 *Although* you could argue that you can always simply open the module
 file first, and that finit_module() is really what we should have had in
 the first place.  Then you don't need the flags since those would come
 from openat().
 
 There's no fundamental reason that modules have to be in a file.  I'm
 thinking of compressed modules, or an initrd which simply includes all
 the modules it wants to load in one linear file.
 
 Also, --force options manipulate the module before loading (as did the
 now-obsolete module rename option).
 

So perhaps what we *should* have is something that points to the module
to a (buffer, length) in userspace, and the equivalent of the current
init_module() would be open() + mmap() + minit_module() + close()?

-hpa



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-17 Thread Lucas De Marchi

On Thu, Oct 18, 2012 at 12:12 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 Michael Kerrisk (man-pages) mtk.manpa...@gmail.com writes:
 Sure. But my point that started this subthread was: should we take the
 opportunity now to add a 'flags' argument to the new finit_module()
 system call, so as to allow flexibility in extending the behavior in
 future? There have been so many cases of revised system calls in the
 past few years that replaced calls without a 'flags' argument that it
 seems worth at least some thought before the API is cast in stone.

 (CC's trimmed, Lucas  Jon added; please include them in module
 discussions!)

 So I tried to think of why we'd want flags; if I could think of a
 plausible reason, obviously we should do it now.

 I think it would be neat for the force flags (eg. ignoring modversions
 or ignoring kernel version).  These are the only cases where libkmod
 needs to mangle the module.

Maybe we should put this back into kernel. With an fd we can't mangle
the module anymore to ignore modversions or kernel version.

So yes, I think a 'flags' param is indeed needed.

Side note:  force won't work anymore by using init_module() and signed modules.


 So here's the patch which adds the flags field, but nothing in there
 yet.  I'll add the remove flags soon, so libkmod can assume that if the
 syscall exists, those flags will work.

 Thoughts?
 Rusty.

 FIX: add flags arg to sys_finit_module()

 Thanks to Michael Kerrisk for keeping us honest.

 diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
 index 32bc035..8cf7b50 100644
 --- a/include/linux/syscalls.h
 +++ b/include/linux/syscalls.h
 @@ -868,5 +868,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,

  asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
  unsigned long idx1, unsigned long idx2);
 -asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 +asmlinkage long sys_finit_module(int fd, const char __user *uargs, int 
 flags);
  #endif
 diff --git a/kernel/module.c b/kernel/module.c
 index 261bf82..8b8d986 100644
 --- a/kernel/module.c
 +++ b/kernel/module.c
 @@ -3260,7 +3260,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 return load_module(info, uargs);
  }

 -SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
 +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs, int, 
 flags)
  {
 int err;
 struct load_info info = { };
 @@ -3269,7 +3269,11 @@ SYSCALL_DEFINE2(finit_module, int, fd, const char 
 __user *, uargs)
 if (err)
 return err;

 -   pr_debug(finit_module: fd=%d, uargs=%p\n, fd, uargs);
 +   pr_debug(finit_module: fd=%d, uargs=%p, flags=%i\n, fd, uargs, 
 flags);
 +
 +   /* Coming RSN... */
 +   if (flags)
 +   return -EINVAL;

 err = copy_module_from_fd(fd, info);
 if (err)


Ack.

Lucas De Marchi
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-11 Thread Michael Kerrisk (man-pages)

Rusty,

On Fri, Oct 12, 2012 at 12:16 AM, Rusty Russell  wrote:
> "H. Peter Anvin"  writes:
>
>> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>>> Good point. A "whole hog" openat()-style interface is worth thinking about 
>>> too.
>>
>> *Although* you could argue that you can always simply open the module
>> file first, and that finit_module() is really what we should have had in
>> the first place.  Then you don't need the flags since those would come
>> from openat().
>
> There's no fundamental reason that modules have to be in a file.  I'm
> thinking of compressed modules, or an initrd which simply includes all
> the modules it wants to load in one linear file.
>
> Also, --force options manipulate the module before loading (as did the
> now-obsolete module rename option).

Sure. But my point that started this subthread was: should we take the
opportunity now to add a 'flags' argument to the new finit_module()
system call, so as to allow flexibility in extending the behavior in
future? There have been so many cases of revised system calls in the
past few years that replaced calls without a 'flags' argument that it
seems worth at least some thought before the API is cast in stone.

Thanks,

Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-11 Thread Rusty Russell

"H. Peter Anvin"  writes:

> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
>> Good point. A "whole hog" openat()-style interface is worth thinking about 
>> too.
>
> *Although* you could argue that you can always simply open the module
> file first, and that finit_module() is really what we should have had in
> the first place.  Then you don't need the flags since those would come
> from openat().

There's no fundamental reason that modules have to be in a file.  I'm
thinking of compressed modules, or an initrd which simply includes all
the modules it wants to load in one linear file.

Also, --force options manipulate the module before loading (as did the
now-obsolete module rename option).

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-11 Thread Rusty Russell

H. Peter Anvin h...@zytor.com writes:

 On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
 Good point. A whole hog openat()-style interface is worth thinking about 
 too.

 *Although* you could argue that you can always simply open the module
 file first, and that finit_module() is really what we should have had in
 the first place.  Then you don't need the flags since those would come
 from openat().

There's no fundamental reason that modules have to be in a file.  I'm
thinking of compressed modules, or an initrd which simply includes all
the modules it wants to load in one linear file.

Also, --force options manipulate the module before loading (as did the
now-obsolete module rename option).

Cheers,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-11 Thread Michael Kerrisk (man-pages)

Rusty,

On Fri, Oct 12, 2012 at 12:16 AM, Rusty Russell ru...@rustcorp.com.au wrote:
 H. Peter Anvin h...@zytor.com writes:

 On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
 Good point. A whole hog openat()-style interface is worth thinking about 
 too.

 *Although* you could argue that you can always simply open the module
 file first, and that finit_module() is really what we should have had in
 the first place.  Then you don't need the flags since those would come
 from openat().

 There's no fundamental reason that modules have to be in a file.  I'm
 thinking of compressed modules, or an initrd which simply includes all
 the modules it wants to load in one linear file.

 Also, --force options manipulate the module before loading (as did the
 now-obsolete module rename option).

Sure. But my point that started this subthread was: should we take the
opportunity now to add a 'flags' argument to the new finit_module()
system call, so as to allow flexibility in extending the behavior in
future? There have been so many cases of revised system calls in the
past few years that replaced calls without a 'flags' argument that it
seems worth at least some thought before the API is cast in stone.

Thanks,

Michael
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

[resending because my mobile device decided it
wanted to send HTML, which of course bounced.]

On Oct 10, 2012 12:09 AM, "H. Peter Anvin"  wrote:
>
> On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
> > Good point. A "whole hog" openat()-style interface is worth thinking about 
> > too.
>
> *Although* you could argue that you can always simply open the module
> file first, and that finit_module() is really what we should have had in
> the first place.  Then you don't need the flags since those would come
> from openat().

But in that case, I'd still stand by my original point: it may be
desirable to have a flags argument to allow future modifications to
the behavior of finit_module() (as opposed to the behavior of the file
open).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
> Good point. A "whole hog" openat()-style interface is worth thinking about 
> too.

*Although* you could argue that you can always simply open the module
file first, and that finit_module() is really what we should have had in
the first place.  Then you don't need the flags since those would come
from openat().

-hpa

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 9, 2012 at 11:58 PM, H. Peter Anvin  wrote:
> On 10/10/2012 05:54 AM, Michael Kerrisk wrote:
>> Kees,
>>
>>> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
>>
>> Given the repeated experience of the last few years--new system calls
>> that are in essence revisions of older system calls with a 'flags'
>> argument bolted on to allow more flexible behavior (e.g., accept4(),
>> dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
>> on.)--maybe it is worth considering adding a 'flags' bit mask
>> argument[1] to the finti_module() system call now, to allow for
>> possible future extensions to the behavior of the interface. What do
>> you think?
>>
>> Thanks,
>>
>> Michael
>>
>> [1] Yes, I know that init_module() doesn't have a flags argument, but
>> that interface was added when we'd seen fewer of the kinds of cases
>> listed above.
>>
>
> Then maybe go whole hog and make it an init_module_at() system call
> (allowing NULL for the pathname half to implement finit_module())...?

Good point. A "whole hog" openat()-style interface is worth thinking about too.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/10/2012 05:54 AM, Michael Kerrisk wrote:
> Kees,
> 
>> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
> 
> Given the repeated experience of the last few years--new system calls
> that are in essence revisions of older system calls with a 'flags'
> argument bolted on to allow more flexible behavior (e.g., accept4(),
> dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
> on.)--maybe it is worth considering adding a 'flags' bit mask
> argument[1] to the finti_module() system call now, to allow for
> possible future extensions to the behavior of the interface. What do
> you think?
> 
> Thanks,
> 
> Michael
> 
> [1] Yes, I know that init_module() doesn't have a flags argument, but
> that interface was added when we'd seen fewer of the kinds of cases
> listed above.
> 

Then maybe go whole hog and make it an init_module_at() system call
(allowing NULL for the pathname half to implement finit_module())...?

-hpa


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-09 Thread Michael Kerrisk

Kees,

> +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)

Given the repeated experience of the last few years--new system calls
that are in essence revisions of older system calls with a 'flags'
argument bolted on to allow more flexible behavior (e.g., accept4(),
dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
on.)--maybe it is worth considering adding a 'flags' bit mask
argument[1] to the finti_module() system call now, to allow for
possible future extensions to the behavior of the interface. What do
you think?

Thanks,

Michael

[1] Yes, I know that init_module() doesn't have a flags argument, but
that interface was added when we'd seen fewer of the kinds of cases
listed above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-09 Thread Michael Kerrisk

Kees,

 +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)

Given the repeated experience of the last few years--new system calls
that are in essence revisions of older system calls with a 'flags'
argument bolted on to allow more flexible behavior (e.g., accept4(),
dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
on.)--maybe it is worth considering adding a 'flags' bit mask
argument[1] to the finti_module() system call now, to allow for
possible future extensions to the behavior of the interface. What do
you think?

Thanks,

Michael

[1] Yes, I know that init_module() doesn't have a flags argument, but
that interface was added when we'd seen fewer of the kinds of cases
listed above.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/10/2012 05:54 AM, Michael Kerrisk wrote:
 Kees,
 
 +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)
 
 Given the repeated experience of the last few years--new system calls
 that are in essence revisions of older system calls with a 'flags'
 argument bolted on to allow more flexible behavior (e.g., accept4(),
 dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
 on.)--maybe it is worth considering adding a 'flags' bit mask
 argument[1] to the finti_module() system call now, to allow for
 possible future extensions to the behavior of the interface. What do
 you think?
 
 Thanks,
 
 Michael
 
 [1] Yes, I know that init_module() doesn't have a flags argument, but
 that interface was added when we'd seen fewer of the kinds of cases
 listed above.
 

Then maybe go whole hog and make it an init_module_at() system call
(allowing NULL for the pathname half to implement finit_module())...?

-hpa


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Tue, Oct 9, 2012 at 11:58 PM, H. Peter Anvin h...@zytor.com wrote:
 On 10/10/2012 05:54 AM, Michael Kerrisk wrote:
 Kees,

 +SYSCALL_DEFINE2(finit_module, int, fd, const char __user *, uargs)

 Given the repeated experience of the last few years--new system calls
 that are in essence revisions of older system calls with a 'flags'
 argument bolted on to allow more flexible behavior (e.g., accept4(),
 dup3(), utimensat(), epoll_create1(), pipe2(), inotify_init(1), and so
 on.)--maybe it is worth considering adding a 'flags' bit mask
 argument[1] to the finti_module() system call now, to allow for
 possible future extensions to the behavior of the interface. What do
 you think?

 Thanks,

 Michael

 [1] Yes, I know that init_module() doesn't have a flags argument, but
 that interface was added when we'd seen fewer of the kinds of cases
 listed above.


 Then maybe go whole hog and make it an init_module_at() system call
 (allowing NULL for the pathname half to implement finit_module())...?

Good point. A whole hog openat()-style interface is worth thinking about too.

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of The Linux Programming Interface; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
 Good point. A whole hog openat()-style interface is worth thinking about 
 too.

*Although* you could argue that you can always simply open the module
file first, and that finit_module() is really what we should have had in
the first place.  Then you don't need the flags since those would come
from openat().

-hpa


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

[resending because my mobile device decided it
wanted to send HTML, which of course bounced.]

On Oct 10, 2012 12:09 AM, H. Peter Anvin h...@zytor.com wrote:

 On 10/10/2012 06:03 AM, Michael Kerrisk (man-pages) wrote:
  Good point. A whole hog openat()-style interface is worth thinking about 
  too.

 *Although* you could argue that you can always simply open the module
 file first, and that finit_module() is really what we should have had in
 the first place.  Then you don't need the flags since those would come
 from openat().

But in that case, I'd still stand by my original point: it may be
desirable to have a flags argument to allow future modifications to
the behavior of finit_module() (as opposed to the behavior of the file
open).
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-05 Thread Rusty Russell

Mimi Zohar  writes:
> Why?  Not only have you had these patches sitting for a while, way
> before you had the kernel module patches, they've been acked/signed off
> by Kees, Serge, Eric, and myself.  All security subtree maintainers.
> The module patches could have easily been built on top of Kees' small
> patches.  I am really disappointed!

Me too.

Linus' merge window opened on Monday 1-10-2012.  One week before that
was Monday 24-09-2012, which is the nominal close of my merge window.

The patches were sent on 21-09 (Friday for you, the weekend my time).

If I had nothing else to do on Monday, I would have applied it, but we
spent the week trying to get the module signing patches into shape :(

If I take them now, they need to go through linux-next.  That won't
happen until Monday.  I want two days in linux-next, so that people who
test linux-next get a chance to find issues, so that's Wednesday before
I send to Linus, which is getting very late into the merge window.

And keep adding two days for every trivial issue which is found :(

It's in my modules-wip branch for 3.8.

Cheers,
Rusty.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-05 Thread Kees Cook

On Thu, Oct 4, 2012 at 8:50 PM, Rusty Russell  wrote:
> Mimi Zohar  writes:
>> Why?  Not only have you had these patches sitting for a while, way
>> before you had the kernel module patches, they've been acked/signed off
>> by Kees, Serge, Eric, and myself.  All security subtree maintainers.
>> The module patches could have easily been built on top of Kees' small
>> patches.  I am really disappointed!
>
> Me too.
>
> Linus' merge window opened on Monday 1-10-2012.  One week before that
> was Monday 24-09-2012, which is the nominal close of my merge window.
>
> The patches were sent on 21-09 (Friday for you, the weekend my time).
>
> If I had nothing else to do on Monday, I would have applied it, but we
> spent the week trying to get the module signing patches into shape :(
>
> If I take them now, they need to go through linux-next.  That won't
> happen until Monday.  I want two days in linux-next, so that people who
> test linux-next get a chance to find issues, so that's Wednesday before
> I send to Linus, which is getting very late into the merge window.
>
> And keep adding two days for every trivial issue which is found :(
>
> It's in my modules-wip branch for 3.8.

Cool; better than not in at all. :) Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-05 Thread Kees Cook

On Thu, Oct 4, 2012 at 8:50 PM, Rusty Russell ru...@rustcorp.com.au wrote:
 Mimi Zohar zo...@linux.vnet.ibm.com writes:
 Why?  Not only have you had these patches sitting for a while, way
 before you had the kernel module patches, they've been acked/signed off
 by Kees, Serge, Eric, and myself.  All security subtree maintainers.
 The module patches could have easily been built on top of Kees' small
 patches.  I am really disappointed!

 Me too.

 Linus' merge window opened on Monday 1-10-2012.  One week before that
 was Monday 24-09-2012, which is the nominal close of my merge window.

 The patches were sent on 21-09 (Friday for you, the weekend my time).

 If I had nothing else to do on Monday, I would have applied it, but we
 spent the week trying to get the module signing patches into shape :(

 If I take them now, they need to go through linux-next.  That won't
 happen until Monday.  I want two days in linux-next, so that people who
 test linux-next get a chance to find issues, so that's Wednesday before
 I send to Linus, which is getting very late into the merge window.

 And keep adding two days for every trivial issue which is found :(

 It's in my modules-wip branch for 3.8.

Cool; better than not in at all. :) Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-05 Thread Rusty Russell

Mimi Zohar zo...@linux.vnet.ibm.com writes:
 Why?  Not only have you had these patches sitting for a while, way
 before you had the kernel module patches, they've been acked/signed off
 by Kees, Serge, Eric, and myself.  All security subtree maintainers.
 The module patches could have easily been built on top of Kees' small
 patches.  I am really disappointed!

Me too.

Linus' merge window opened on Monday 1-10-2012.  One week before that
was Monday 24-09-2012, which is the nominal close of my merge window.

The patches were sent on 21-09 (Friday for you, the weekend my time).

If I had nothing else to do on Monday, I would have applied it, but we
spent the week trying to get the module signing patches into shape :(

If I take them now, they need to go through linux-next.  That won't
happen until Monday.  I want two days in linux-next, so that people who
test linux-next get a chance to find issues, so that's Wednesday before
I send to Linus, which is getting very late into the merge window.

And keep adding two days for every trivial issue which is found :(

It's in my modules-wip branch for 3.8.

Cheers,
Rusty.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Wed, Oct 3, 2012 at 10:39 PM, Rusty Russell  wrote:
> Kees Cook  writes:
>
>> On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook  wrote:
>>> As part of the effort to create a stronger boundary between root and
>>> kernel, Chrome OS wants to be able to enforce that kernel modules are
>>> being loaded only from our read-only crypto-hash verified (dm_verity)
>>> root filesystem. Since the init_module syscall hands the kernel a module
>>> as a memory blob, no reasoning about the origin of the blob can be made.
>>>
>>> Earlier proposals for appending signatures to kernel modules would not be
>>> useful in Chrome OS, since it would involve adding an additional set of
>>> keys to our kernel and builds for no good reason: we already trust the
>>> contents of our root filesystem. We don't need to verify those kernel
>>> modules a second time. Having to do signature checking on module loading
>>> would slow us down and be redundant. All we need to know is where a
>>> module is coming from so we can say yes/no to loading it.
>>>
>>> If a file descriptor is used as the source of a kernel module, many more
>>> things can be reasoned about. In Chrome OS's case, we could enforce that
>>> the module lives on the filesystem we expect it to live on.  In the case
>>> of IMA (or other LSMs), it would be possible, for example, to examine
>>> extended attributes that may contain signatures over the contents of
>>> the module.
>>>
>>> This introduces a new syscall (on x86), similar to init_module, that has
>>> only two arguments. The first argument is used as a file descriptor to
>>> the module and the second argument is a pointer to the NULL terminated
>>> string of module arguments.
>>
>> Hi Rusty,
>>
>> Is this likely to land in the 3.7 change window? I'd really like to
>> get the syscall number assigned so I can start sending patches to
>> glibc, kmod, etc. My tree is here, FWIW:
>
> No, unfortunately it's a little late and there were issues with ARM
> signoffs and syscall numbers...
>
>> http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall
>
> Messy merge due to the module signing stuff going in :(

Sure was! :) I've done the merge now (and sent the v5 patches). I
think it looks pretty clean now.

> Please rebase on top of my kernel.org modules-next branch, and I'll pull
> into my modules-wip branch for 3.8.

As Mimi mentioned, it would be really nice if this could land in 3.7.
Can I maybe convince you? It's technically a small change, just with a
lot of reordering of the calling code, but I think it's a relatively
small change. The diff output is horrible due to extracting
do_init_module, but the code changed is pretty minimal.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/4] module: add syscall to load module from fd

As part of the effort to create a stronger boundary between root and
kernel, Chrome OS wants to be able to enforce that kernel modules are
being loaded only from our read-only crypto-hash verified (dm_verity)
root filesystem. Since the init_module syscall hands the kernel a module
as a memory blob, no reasoning about the origin of the blob can be made.

Earlier proposals for appending signatures to kernel modules would not be
useful in Chrome OS, since it would involve adding an additional set of
keys to our kernel and builds for no good reason: we already trust the
contents of our root filesystem. We don't need to verify those kernel
modules a second time. Having to do signature checking on module loading
would slow us down and be redundant. All we need to know is where a
module is coming from so we can say yes/no to loading it.

If a file descriptor is used as the source of a kernel module, many more
things can be reasoned about. In Chrome OS's case, we could enforce that
the module lives on the filesystem we expect it to live on.  In the case
of IMA (or other LSMs), it would be possible, for example, to examine
extended attributes that may contain signatures over the contents of
the module.

This introduces a new syscall (on x86), similar to init_module, that has
only two arguments. The first argument is used as a file descriptor to
the module and the second argument is a pointer to the NULL terminated
string of module arguments.

Signed-off-by: Kees Cook 
Cc: Rusty Russell 
Cc: Andrew Morton 
---
v5:
 - rebase onto rusty's module-next tree.
v4:
 - fixed misplaced ack (moved from ARM to asm-generic).
v3:
 - fix ret from copy_from_user, thanks to Fengguang Wu.
 - rename to finit_module, suggested by Peter Anvin.
 - various cleanups, suggested from Andrew Morton.
v2:
 - various cleanups, thanks to Rusty Russell.
---
 arch/x86/syscalls/syscall_32.tbl |1 +
 arch/x86/syscalls/syscall_64.tbl |1 +
 include/linux/syscalls.h |1 +
 kernel/module.c  |  363 +++---
 kernel/sys_ni.c  |1 +
 5 files changed, 222 insertions(+), 145 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..74ccf44 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -356,3 +356,4 @@
 347i386process_vm_readvsys_process_vm_readv
compat_sys_process_vm_readv
 348i386process_vm_writev   sys_process_vm_writev   
compat_sys_process_vm_writev
 349i386kcmpsys_kcmp
+350i386finit_modulesys_finit_module
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..7c58c84 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -319,6 +319,7 @@
 31064  process_vm_readvsys_process_vm_readv
 31164  process_vm_writev   sys_process_vm_writev
 312common  kcmpsys_kcmp
+313common  finit_modulesys_finit_module
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 19439c7..a377796 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -860,4 +860,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 0e2da86..321c6b9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2420,12 +2421,12 @@ static inline void kmemleak_load_module(const struct 
module *mod,
 #endif
 
 #ifdef CONFIG_MODULE_SIG
-static int module_sig_check(struct load_info *info,
-   const void *mod, unsigned long *len)
+static int module_sig_check(struct load_info *info)
 {
int err = -ENOKEY;
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
-   const void *p = mod, *end = mod + *len;
+   const void *mod = info->hdr;
+   const void *p = mod, *end = mod + info->len;
 
/* Poor man's memmem. */
while ((p = memchr(p, MODULE_SIG_STRING[0], end - p))) {
@@ -2435,8 +2436,9 @@ static int module_sig_check(struct load_info *info,
if (memcmp(p, MODULE_SIG_STRING, markerlen) == 0) {
const void *sig = p + markerlen;
/* Truncate module up to signature. */
-   *len = p - mod;
-   err = mod_verify_sig(mod, *len, sig, end - sig);
+   info->len = p - mod;
+   err = mod_verify_sig(mod, info->len,
+

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-04 Thread Mimi Zohar

On Thu, 2012-10-04 at 15:09 +0930, Rusty Russell wrote:
> Kees Cook  writes:
> 
> > On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook  wrote:
> >> As part of the effort to create a stronger boundary between root and
> >> kernel, Chrome OS wants to be able to enforce that kernel modules are
> >> being loaded only from our read-only crypto-hash verified (dm_verity)
> >> root filesystem. Since the init_module syscall hands the kernel a module
> >> as a memory blob, no reasoning about the origin of the blob can be made.
> >>
> >> Earlier proposals for appending signatures to kernel modules would not be
> >> useful in Chrome OS, since it would involve adding an additional set of
> >> keys to our kernel and builds for no good reason: we already trust the
> >> contents of our root filesystem. We don't need to verify those kernel
> >> modules a second time. Having to do signature checking on module loading
> >> would slow us down and be redundant. All we need to know is where a
> >> module is coming from so we can say yes/no to loading it.
> >>
> >> If a file descriptor is used as the source of a kernel module, many more
> >> things can be reasoned about. In Chrome OS's case, we could enforce that
> >> the module lives on the filesystem we expect it to live on.  In the case
> >> of IMA (or other LSMs), it would be possible, for example, to examine
> >> extended attributes that may contain signatures over the contents of
> >> the module.
> >>
> >> This introduces a new syscall (on x86), similar to init_module, that has
> >> only two arguments. The first argument is used as a file descriptor to
> >> the module and the second argument is a pointer to the NULL terminated
> >> string of module arguments.
> >
> > Hi Rusty,
> >
> > Is this likely to land in the 3.7 change window? I'd really like to
> > get the syscall number assigned so I can start sending patches to
> > glibc, kmod, etc. My tree is here, FWIW:
> 
> No, unfortunately it's a little late and there were issues with ARM
> signoffs and syscall numbers...
> 
> > http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall
> 
> Messy merge due to the module signing stuff going in :(
> 
> Please rebase on top of my kernel.org modules-next branch, and I'll pull
> into my modules-wip branch for 3.8.

Why?  Not only have you had these patches sitting for a while, way
before you had the kernel module patches, they've been acked/signed off
by Kees, Serge, Eric, and myself.  All security subtree maintainers.
The module patches could have easily been built on top of Kees' small
patches.  I am really disappointed!

Mimi

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-04 Thread Mimi Zohar

On Thu, 2012-10-04 at 15:09 +0930, Rusty Russell wrote:
Kees Cook keesc...@chromium.org writes:

On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook keesc...@chromium.org wrote:
As part of the effort to create a stronger boundary between root and
kernel, Chrome OS wants to be able to enforce that kernel modules are
being loaded only from our read-only crypto-hash verified (dm_verity)
root filesystem. Since the init_module syscall hands the kernel a module
as a memory blob, no reasoning about the origin of the blob can be made.

Earlier proposals for appending signatures to kernel modules would not be
useful in Chrome OS, since it would involve adding an additional set of
keys to our kernel and builds for no good reason: we already trust the
contents of our root filesystem. We don't need to verify those kernel
modules a second time. Having to do signature checking on module loading
would slow us down and be redundant. All we need to know is where a
module is coming from so we can say yes/no to loading it.

If a file descriptor is used as the source of a kernel module, many more
things can be reasoned about. In Chrome OS's case, we could enforce that
the module lives on the filesystem we expect it to live on. In the case
of IMA (or other LSMs), it would be possible, for example, to examine
extended attributes that may contain signatures over the contents of
the module.

This introduces a new syscall (on x86), similar to init_module, that has
only two arguments. The first argument is used as a file descriptor to
the module and the second argument is a pointer to the NULL terminated
string of module arguments.

Hi Rusty,

Is this likely to land in the 3.7 change window? I'd really like to
get the syscall number assigned so I can start sending patches to
glibc, kmod, etc. My tree is here, FWIW:

No, unfortunately it's a little late and there were issues with ARM
signoffs and syscall numbers...

http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Messy merge due to the module signing stuff going in :(

Please rebase on top of my kernel.org modules-next branch, and I'll pull
into my modules-wip branch for 3.8.

Why? Not only have you had these patches sitting for a while, way
before you had the kernel module patches, they've been acked/signed off
by Kees, Serge, Eric, and myself. All security subtree maintainers.
The module patches could have easily been built on top of Kees' small
patches. I am really disappointed!

Mimi

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[PATCH 1/4] module: add syscall to load module from fd

As part of the effort to create a stronger boundary between root and
kernel, Chrome OS wants to be able to enforce that kernel modules are
being loaded only from our read-only crypto-hash verified (dm_verity)
root filesystem. Since the init_module syscall hands the kernel a module
as a memory blob, no reasoning about the origin of the blob can be made.

Earlier proposals for appending signatures to kernel modules would not be
useful in Chrome OS, since it would involve adding an additional set of
keys to our kernel and builds for no good reason: we already trust the
contents of our root filesystem. We don't need to verify those kernel
modules a second time. Having to do signature checking on module loading
would slow us down and be redundant. All we need to know is where a
module is coming from so we can say yes/no to loading it.

If a file descriptor is used as the source of a kernel module, many more
things can be reasoned about. In Chrome OS's case, we could enforce that
the module lives on the filesystem we expect it to live on.  In the case
of IMA (or other LSMs), it would be possible, for example, to examine
extended attributes that may contain signatures over the contents of
the module.

This introduces a new syscall (on x86), similar to init_module, that has
only two arguments. The first argument is used as a file descriptor to
the module and the second argument is a pointer to the NULL terminated
string of module arguments.

Signed-off-by: Kees Cook keesc...@chromium.org
Cc: Rusty Russell ru...@rustcorp.com.au
Cc: Andrew Morton a...@linux-foundation.org
---
v5:
 - rebase onto rusty's module-next tree.
v4:
 - fixed misplaced ack (moved from ARM to asm-generic).
v3:
 - fix ret from copy_from_user, thanks to Fengguang Wu.
 - rename to finit_module, suggested by Peter Anvin.
 - various cleanups, suggested from Andrew Morton.
v2:
 - various cleanups, thanks to Rusty Russell.
---
 arch/x86/syscalls/syscall_32.tbl |1 +
 arch/x86/syscalls/syscall_64.tbl |1 +
 include/linux/syscalls.h |1 +
 kernel/module.c  |  363 +++---
 kernel/sys_ni.c  |1 +
 5 files changed, 222 insertions(+), 145 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..74ccf44 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -356,3 +356,4 @@
 347i386process_vm_readvsys_process_vm_readv
compat_sys_process_vm_readv
 348i386process_vm_writev   sys_process_vm_writev   
compat_sys_process_vm_writev
 349i386kcmpsys_kcmp
+350i386finit_modulesys_finit_module
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..7c58c84 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -319,6 +319,7 @@
 31064  process_vm_readvsys_process_vm_readv
 31164  process_vm_writev   sys_process_vm_writev
 312common  kcmpsys_kcmp
+313common  finit_modulesys_finit_module
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 19439c7..a377796 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -860,4 +860,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 0e2da86..321c6b9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -21,6 +21,7 @@
 #include linux/ftrace_event.h
 #include linux/init.h
 #include linux/kallsyms.h
+#include linux/file.h
 #include linux/fs.h
 #include linux/sysfs.h
 #include linux/kernel.h
@@ -2420,12 +2421,12 @@ static inline void kmemleak_load_module(const struct 
module *mod,
 #endif
 
 #ifdef CONFIG_MODULE_SIG
-static int module_sig_check(struct load_info *info,
-   const void *mod, unsigned long *len)
+static int module_sig_check(struct load_info *info)
 {
int err = -ENOKEY;
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
-   const void *p = mod, *end = mod + *len;
+   const void *mod = info-hdr;
+   const void *p = mod, *end = mod + info-len;
 
/* Poor man's memmem. */
while ((p = memchr(p, MODULE_SIG_STRING[0], end - p))) {
@@ -2435,8 +2436,9 @@ static int module_sig_check(struct load_info *info,
if (memcmp(p, MODULE_SIG_STRING, markerlen) == 0) {
const void *sig = p + markerlen;
/* Truncate module up to signature. */
-   *len = p - mod;
-   err = mod_verify_sig(mod, *len, sig,

Re: [PATCH 1/4] module: add syscall to load module from fd

On Wed, Oct 3, 2012 at 10:39 PM, Rusty Russell ru...@rustcorp.com.au wrote:
Kees Cook keesc...@chromium.org writes:

Hi Rusty,

Is this likely to land in the 3.7 change window? I'd really like to
get the syscall number assigned so I can start sending patches to
glibc, kmod, etc. My tree is here, FWIW:

No, unfortunately it's a little late and there were issues with ARM
signoffs and syscall numbers...

http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Messy merge due to the module signing stuff going in :(

Sure was! :) I've done the merge now (and sent the v5 patches). I
think it looks pretty clean now.

Please rebase on top of my kernel.org modules-next branch, and I'll pull
into my modules-wip branch for 3.8.

As Mimi mentioned, it would be really nice if this could land in 3.7.
Can I maybe convince you? It's technically a small change, just with a
lot of reordering of the calling code, but I think it's a relatively
small change. The diff output is horrible due to extracting
do_init_module, but the code changed is pretty minimal.

-Kees

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-03 Thread Rusty Russell

Kees Cook  writes:

> On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook  wrote:
>> As part of the effort to create a stronger boundary between root and
>> kernel, Chrome OS wants to be able to enforce that kernel modules are
>> being loaded only from our read-only crypto-hash verified (dm_verity)
>> root filesystem. Since the init_module syscall hands the kernel a module
>> as a memory blob, no reasoning about the origin of the blob can be made.
>>
>> Earlier proposals for appending signatures to kernel modules would not be
>> useful in Chrome OS, since it would involve adding an additional set of
>> keys to our kernel and builds for no good reason: we already trust the
>> contents of our root filesystem. We don't need to verify those kernel
>> modules a second time. Having to do signature checking on module loading
>> would slow us down and be redundant. All we need to know is where a
>> module is coming from so we can say yes/no to loading it.
>>
>> If a file descriptor is used as the source of a kernel module, many more
>> things can be reasoned about. In Chrome OS's case, we could enforce that
>> the module lives on the filesystem we expect it to live on.  In the case
>> of IMA (or other LSMs), it would be possible, for example, to examine
>> extended attributes that may contain signatures over the contents of
>> the module.
>>
>> This introduces a new syscall (on x86), similar to init_module, that has
>> only two arguments. The first argument is used as a file descriptor to
>> the module and the second argument is a pointer to the NULL terminated
>> string of module arguments.
>
> Hi Rusty,
>
> Is this likely to land in the 3.7 change window? I'd really like to
> get the syscall number assigned so I can start sending patches to
> glibc, kmod, etc. My tree is here, FWIW:

No, unfortunately it's a little late and there were issues with ARM
signoffs and syscall numbers...

> http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Messy merge due to the module signing stuff going in :(

Please rebase on top of my kernel.org modules-next branch, and I'll pull
into my modules-wip branch for 3.8.

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-03 Thread Kees Cook

On Thu, Sep 20, 2012 at 3:14 PM, Kees Cook  wrote:
> As part of the effort to create a stronger boundary between root and
> kernel, Chrome OS wants to be able to enforce that kernel modules are
> being loaded only from our read-only crypto-hash verified (dm_verity)
> root filesystem. Since the init_module syscall hands the kernel a module
> as a memory blob, no reasoning about the origin of the blob can be made.
>
> Earlier proposals for appending signatures to kernel modules would not be
> useful in Chrome OS, since it would involve adding an additional set of
> keys to our kernel and builds for no good reason: we already trust the
> contents of our root filesystem. We don't need to verify those kernel
> modules a second time. Having to do signature checking on module loading
> would slow us down and be redundant. All we need to know is where a
> module is coming from so we can say yes/no to loading it.
>
> If a file descriptor is used as the source of a kernel module, many more
> things can be reasoned about. In Chrome OS's case, we could enforce that
> the module lives on the filesystem we expect it to live on.  In the case
> of IMA (or other LSMs), it would be possible, for example, to examine
> extended attributes that may contain signatures over the contents of
> the module.
>
> This introduces a new syscall (on x86), similar to init_module, that has
> only two arguments. The first argument is used as a file descriptor to
> the module and the second argument is a pointer to the NULL terminated
> string of module arguments.

Hi Rusty,

Is this likely to land in the 3.7 change window? I'd really like to
get the syscall number assigned so I can start sending patches to
glibc, kmod, etc. My tree is here, FWIW:

http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-03 Thread Kees Cook

Hi Rusty,

Is this likely to land in the 3.7 change window? I'd really like to
get the syscall number assigned so I can start sending patches to
glibc, kmod, etc. My tree is here, FWIW:

http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Thanks!

-Kees

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-10-03 Thread Rusty Russell

Kees Cook keesc...@chromium.org writes:

Hi Rusty,

Is this likely to land in the 3.7 change window? I'd really like to
get the syscall number assigned so I can start sending patches to
glibc, kmod, etc. My tree is here, FWIW:

No, unfortunately it's a little late and there were issues with ARM
signoffs and syscall numbers...

http://git.kernel.org/?p=linux/kernel/git/kees/linux.git;a=shortlog;h=refs/heads/module-fd-syscall

Messy merge due to the module signing stuff going in :(

Please rebase on top of my kernel.org modules-next branch, and I'll pull
into my modules-wip branch for 3.8.

Thanks,
Rusty.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-09-21 Thread John Johansen

On 09/20/2012 07:22 PM, James Morris wrote:
> On Thu, 20 Sep 2012, Kees Cook wrote:
> 
>> Earlier proposals for appending signatures to kernel modules would not be
>> useful in Chrome OS, since it would involve adding an additional set of
>> keys to our kernel and builds for no good reason: we already trust the
>> contents of our root filesystem. We don't need to verify those kernel
>> modules a second time. Having to do signature checking on module loading
>> would slow us down and be redundant. All we need to know is where a
>> module is coming from so we can say yes/no to loading it.
> 
> Just out of interest, has anyone else expressed interest in using this 
> feature?
> 
we are looking at using it in apparmor as well

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-09-21 Thread John Johansen

On 09/20/2012 07:22 PM, James Morris wrote:
 On Thu, 20 Sep 2012, Kees Cook wrote:
 
 Earlier proposals for appending signatures to kernel modules would not be
 useful in Chrome OS, since it would involve adding an additional set of
 keys to our kernel and builds for no good reason: we already trust the
 contents of our root filesystem. We don't need to verify those kernel
 modules a second time. Having to do signature checking on module loading
 would slow us down and be redundant. All we need to know is where a
 module is coming from so we can say yes/no to loading it.
 
 Just out of interest, has anyone else expressed interest in using this 
 feature?
 
we are looking at using it in apparmor as well

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-09-20 Thread Mimi Zohar

On Fri, 2012-09-21 at 12:22 +1000, James Morris wrote:
> On Thu, 20 Sep 2012, Kees Cook wrote:
> 
> > Earlier proposals for appending signatures to kernel modules would not be
> > useful in Chrome OS, since it would involve adding an additional set of
> > keys to our kernel and builds for no good reason: we already trust the
> > contents of our root filesystem. We don't need to verify those kernel
> > modules a second time. Having to do signature checking on module loading
> > would slow us down and be redundant. All we need to know is where a
> > module is coming from so we can say yes/no to loading it.
> 
> Just out of interest, has anyone else expressed interest in using this 
> feature?

I'm not so interested in this particular use case, but am interested in
using the new syscall's file descriptor for measuring/appraising a
kernel module's integrity.

thanks,

Mimi


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

On Thu, Sep 20, 2012 at 7:22 PM, James Morris  wrote:
> On Thu, 20 Sep 2012, Kees Cook wrote:
>
>> Earlier proposals for appending signatures to kernel modules would not be
>> useful in Chrome OS, since it would involve adding an additional set of
>> keys to our kernel and builds for no good reason: we already trust the
>> contents of our root filesystem. We don't need to verify those kernel
>> modules a second time. Having to do signature checking on module loading
>> would slow us down and be redundant. All we need to know is where a
>> module is coming from so we can say yes/no to loading it.
>
> Just out of interest, has anyone else expressed interest in using this
> feature?

Yes, in the earlier threads, Mimi spoke up in favor of it as a
possible path for IMA to do signature checking. She sent patches that
updated the LSM hooks to include callback to IMA that were sent to the
lsm list:
http://marc.info/?l=linux-security-module=134739023306344=2

Serge and Eric both Acked the new hooks too.

-Kees

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-09-20 Thread James Morris

On Thu, 20 Sep 2012, Kees Cook wrote:

> Earlier proposals for appending signatures to kernel modules would not be
> useful in Chrome OS, since it would involve adding an additional set of
> keys to our kernel and builds for no good reason: we already trust the
> contents of our root filesystem. We don't need to verify those kernel
> modules a second time. Having to do signature checking on module loading
> would slow us down and be redundant. All we need to know is where a
> module is coming from so we can say yes/no to loading it.

Just out of interest, has anyone else expressed interest in using this 
feature?



-- 
James Morris

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/4] module: add syscall to load module from fd

As part of the effort to create a stronger boundary between root and
kernel, Chrome OS wants to be able to enforce that kernel modules are
being loaded only from our read-only crypto-hash verified (dm_verity)
root filesystem. Since the init_module syscall hands the kernel a module
as a memory blob, no reasoning about the origin of the blob can be made.

Earlier proposals for appending signatures to kernel modules would not be
useful in Chrome OS, since it would involve adding an additional set of
keys to our kernel and builds for no good reason: we already trust the
contents of our root filesystem. We don't need to verify those kernel
modules a second time. Having to do signature checking on module loading
would slow us down and be redundant. All we need to know is where a
module is coming from so we can say yes/no to loading it.

If a file descriptor is used as the source of a kernel module, many more
things can be reasoned about. In Chrome OS's case, we could enforce that
the module lives on the filesystem we expect it to live on.  In the case
of IMA (or other LSMs), it would be possible, for example, to examine
extended attributes that may contain signatures over the contents of
the module.

This introduces a new syscall (on x86), similar to init_module, that has
only two arguments. The first argument is used as a file descriptor to
the module and the second argument is a pointer to the NULL terminated
string of module arguments.

Signed-off-by: Kees Cook 
---
v3:
 - fix ret from copy_from_user, thanks to Fengguang Wu.
 - rename to finit_module, suggested by Peter Anvin.
 - various cleanups, suggested from Andrew Morton.
v2:
 - various cleanups, thanks to Rusty Russell.
---
 arch/x86/syscalls/syscall_32.tbl |1 +
 arch/x86/syscalls/syscall_64.tbl |1 +
 include/linux/syscalls.h |1 +
 kernel/module.c  |  343 +++--
 kernel/sys_ni.c  |1 +
 5 files changed, 217 insertions(+), 130 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..74ccf44 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -356,3 +356,4 @@
 347i386process_vm_readvsys_process_vm_readv
compat_sys_process_vm_readv
 348i386process_vm_writev   sys_process_vm_writev   
compat_sys_process_vm_writev
 349i386kcmpsys_kcmp
+350i386finit_modulesys_finit_module
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..7c58c84 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -319,6 +319,7 @@
 31064  process_vm_readvsys_process_vm_readv
 31164  process_vm_writev   sys_process_vm_writev
 312common  kcmpsys_kcmp
+313common  finit_modulesys_finit_module
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 19439c7..a377796 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -860,4 +860,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 4edbd9c..afe2f69 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -2399,48 +2400,105 @@ static inline void kmemleak_load_module(const struct 
module *mod,
 }
 #endif
 
+/* Sanity checks against invalid binaries, wrong arch, weird elf version. */
+static int check_info(struct load_info *info)
+{
+   if (info->len < sizeof(*(info->hdr)))
+   return -ENOEXEC;
+
+   if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
+   || info->hdr->e_type != ET_REL
+   || !elf_check_arch(info->hdr)
+   || info->hdr->e_shentsize != sizeof(Elf_Shdr))
+   return -ENOEXEC;
+
+   if (info->hdr->e_shoff >= info->len
+   || (info->hdr->e_shnum * sizeof(Elf_Shdr) >
+   info->len - info->hdr->e_shoff))
+   return -ENOEXEC;
+
+   return 0;
+}
+
 /* Sets info->hdr and info->len. */
-static int copy_and_check(struct load_info *info,
- const void __user *umod, unsigned long len,
- const char __user *uargs)
+static int copy_module_from_user(const void __user *umod, unsigned long len,
+ struct load_info *info)
 {
int err;
-   Elf_Ehdr *hdr;
 
-   if (len < sizeof(*hdr))
+   info->len = len;
+   if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
 
/* Suck

[PATCH 1/4] module: add syscall to load module from fd

As part of the effort to create a stronger boundary between root and
kernel, Chrome OS wants to be able to enforce that kernel modules are
being loaded only from our read-only crypto-hash verified (dm_verity)
root filesystem. Since the init_module syscall hands the kernel a module
as a memory blob, no reasoning about the origin of the blob can be made.

Earlier proposals for appending signatures to kernel modules would not be
useful in Chrome OS, since it would involve adding an additional set of
keys to our kernel and builds for no good reason: we already trust the
contents of our root filesystem. We don't need to verify those kernel
modules a second time. Having to do signature checking on module loading
would slow us down and be redundant. All we need to know is where a
module is coming from so we can say yes/no to loading it.

If a file descriptor is used as the source of a kernel module, many more
things can be reasoned about. In Chrome OS's case, we could enforce that
the module lives on the filesystem we expect it to live on.  In the case
of IMA (or other LSMs), it would be possible, for example, to examine
extended attributes that may contain signatures over the contents of
the module.

This introduces a new syscall (on x86), similar to init_module, that has
only two arguments. The first argument is used as a file descriptor to
the module and the second argument is a pointer to the NULL terminated
string of module arguments.

Signed-off-by: Kees Cook keesc...@chromium.org
---
v3:
 - fix ret from copy_from_user, thanks to Fengguang Wu.
 - rename to finit_module, suggested by Peter Anvin.
 - various cleanups, suggested from Andrew Morton.
v2:
 - various cleanups, thanks to Rusty Russell.
---
 arch/x86/syscalls/syscall_32.tbl |1 +
 arch/x86/syscalls/syscall_64.tbl |1 +
 include/linux/syscalls.h |1 +
 kernel/module.c  |  343 +++--
 kernel/sys_ni.c  |1 +
 5 files changed, 217 insertions(+), 130 deletions(-)

diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl
index 7a35a6e..74ccf44 100644
--- a/arch/x86/syscalls/syscall_32.tbl
+++ b/arch/x86/syscalls/syscall_32.tbl
@@ -356,3 +356,4 @@
 347i386process_vm_readvsys_process_vm_readv
compat_sys_process_vm_readv
 348i386process_vm_writev   sys_process_vm_writev   
compat_sys_process_vm_writev
 349i386kcmpsys_kcmp
+350i386finit_modulesys_finit_module
diff --git a/arch/x86/syscalls/syscall_64.tbl b/arch/x86/syscalls/syscall_64.tbl
index a582bfe..7c58c84 100644
--- a/arch/x86/syscalls/syscall_64.tbl
+++ b/arch/x86/syscalls/syscall_64.tbl
@@ -319,6 +319,7 @@
 31064  process_vm_readvsys_process_vm_readv
 31164  process_vm_writev   sys_process_vm_writev
 312common  kcmpsys_kcmp
+313common  finit_modulesys_finit_module
 
 #
 # x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 19439c7..a377796 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -860,4 +860,5 @@ asmlinkage long sys_process_vm_writev(pid_t pid,
 
 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
 unsigned long idx1, unsigned long idx2);
+asmlinkage long sys_finit_module(int fd, const char __user *uargs);
 #endif
diff --git a/kernel/module.c b/kernel/module.c
index 4edbd9c..afe2f69 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -21,6 +21,7 @@
 #include linux/ftrace_event.h
 #include linux/init.h
 #include linux/kallsyms.h
+#include linux/file.h
 #include linux/fs.h
 #include linux/sysfs.h
 #include linux/kernel.h
@@ -2399,48 +2400,105 @@ static inline void kmemleak_load_module(const struct 
module *mod,
 }
 #endif
 
+/* Sanity checks against invalid binaries, wrong arch, weird elf version. */
+static int check_info(struct load_info *info)
+{
+   if (info-len  sizeof(*(info-hdr)))
+   return -ENOEXEC;
+
+   if (memcmp(info-hdr-e_ident, ELFMAG, SELFMAG) != 0
+   || info-hdr-e_type != ET_REL
+   || !elf_check_arch(info-hdr)
+   || info-hdr-e_shentsize != sizeof(Elf_Shdr))
+   return -ENOEXEC;
+
+   if (info-hdr-e_shoff = info-len
+   || (info-hdr-e_shnum * sizeof(Elf_Shdr) 
+   info-len - info-hdr-e_shoff))
+   return -ENOEXEC;
+
+   return 0;
+}
+
 /* Sets info-hdr and info-len. */
-static int copy_and_check(struct load_info *info,
- const void __user *umod, unsigned long len,
- const char __user *uargs)
+static int copy_module_from_user(const void __user *umod, unsigned long len,
+ struct load_info *info)
 {
int err;
-   Elf_Ehdr *hdr;
 
-   if (len  sizeof(*hdr))
+   info-len = len;
+

Re: [PATCH 1/4] module: add syscall to load module from fd

2012-09-20 Thread James Morris

On Thu, 20 Sep 2012, Kees Cook wrote:

 Earlier proposals for appending signatures to kernel modules would not be
 useful in Chrome OS, since it would involve adding an additional set of
 keys to our kernel and builds for no good reason: we already trust the
 contents of our root filesystem. We don't need to verify those kernel
 modules a second time. Having to do signature checking on module loading
 would slow us down and be redundant. All we need to know is where a
 module is coming from so we can say yes/no to loading it.

Just out of interest, has anyone else expressed interest in using this 
feature?



-- 
James Morris
jmor...@namei.org
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] module: add syscall to load module from fd