diff mbox series

[4/4] module: Add hook for security_kernel_post_read_file()

Message ID 20200707081926.3688096-5-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series Fix misused kernel_read_file() enums | expand

Commit Message

Kees Cook July 7, 2020, 8:19 a.m. UTC
Calls to security_kernel_load_data() should be paired with a call to
security_kernel_post_read_file() with a NULL file argument. Add the
missing call so the module contents are visible to the LSMs interested
in measuring the module content. (This also paves the way for moving
module signature checking out of the module core and into an LSM.)

Cc: Jessica Yu <jeyu@kernel.org>
Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/module.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Mimi Zohar July 8, 2020, 12:47 a.m. UTC | #1
On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> Calls to security_kernel_load_data() should be paired with a call to
> security_kernel_post_read_file() with a NULL file argument. Add the
> missing call so the module contents are visible to the LSMs interested
> in measuring the module content. (This also paves the way for moving
> module signature checking out of the module core and into an LSM.)
> 
> Cc: Jessica Yu <jeyu@kernel.org>
> Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/module.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 0c6573b98c36..af9679f8e5c6 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>  		return -EFAULT;
>  	}
>  
> -	return 0;
> +	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> +					     info->len, READING_MODULE);

There was a lot of push back on calling security_kernel_read_file()
with a NULL file descriptor here.[1]  The result was defining a new
security hook - security_kernel_load_data - and enumeration -
LOADING_MODULE.  I would prefer calling the same pre and post security
hook.

Mimi

[1] http://kernsec.org/pipermail/linux-security-module-archive/2018-Ma
y/007110.html

> +	if (err)
> +		vfree(info->hdr);
> +
> +	return err;
>  }
>  
>  static void free_copy(struct load_info *info)
Kees Cook July 8, 2020, 3:10 a.m. UTC | #2
On Tue, Jul 07, 2020 at 08:47:20PM -0400, Mimi Zohar wrote:
> On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > Calls to security_kernel_load_data() should be paired with a call to
> > security_kernel_post_read_file() with a NULL file argument. Add the
> > missing call so the module contents are visible to the LSMs interested
> > in measuring the module content. (This also paves the way for moving
> > module signature checking out of the module core and into an LSM.)
> > 
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  kernel/module.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 0c6573b98c36..af9679f8e5c6 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> >  		return -EFAULT;
> >  	}
> >  
> > -	return 0;
> > +	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> > +					     info->len, READING_MODULE);
> 
> There was a lot of push back on calling security_kernel_read_file()
> with a NULL file descriptor here.[1]  The result was defining a new
> security hook - security_kernel_load_data - and enumeration -
> LOADING_MODULE.  I would prefer calling the same pre and post security
> hook.
> 
> Mimi
> 
> [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/007110.html

Ah yes, thanks for the pointer to the discussion.

I think we have four cases then, for differing LSM hooks:

- no "file", no contents
	e.g. init_module() before copying user buffer
	security_kernel_load_data()
- only a "file" available, no contents
	e.g. kernel_read_file() before actually reading anything
	security_kernel_read_file()
- "file" and contents
	e.g. kernel_read_file() after reading
	security_kernel_post_read_file()
- no "file" available, just the contents
	e.g. firmware platform fallback from EFI space (no "file")
	unimplemented!

If an LSM wants to be able to examine the contents of firmware, modules,
kexec, etc, it needs either a "file" or the full contents.

The "file" methods all pass through the kernel_read_file()-family. The
others happen via blobs coming from userspace or (more recently) the EFI
universe.

So, if a NULL file is unreasonable, we need, perhaps,
security_kernel_post_load_data()

?
Mimi Zohar July 8, 2020, 1:47 p.m. UTC | #3
On Tue, 2020-07-07 at 20:10 -0700, Kees Cook wrote:
> On Tue, Jul 07, 2020 at 08:47:20PM -0400, Mimi Zohar wrote:
> > On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > > Calls to security_kernel_load_data() should be paired with a call to
> > > security_kernel_post_read_file() with a NULL file argument. Add the
> > > missing call so the module contents are visible to the LSMs interested
> > > in measuring the module content. (This also paves the way for moving
> > > module signature checking out of the module core and into an LSM.)
> > > 
> > > Cc: Jessica Yu <jeyu@kernel.org>
> > > Fixes: c77b8cdf745d ("module: replace the existing LSM hook in init_module")
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > >  kernel/module.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/module.c b/kernel/module.c
> > > index 0c6573b98c36..af9679f8e5c6 100644
> > > --- a/kernel/module.c
> > > +++ b/kernel/module.c
> > > @@ -2980,7 +2980,12 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> > >  		return -EFAULT;
> > >  	}
> > >  
> > > -	return 0;
> > > +	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
> > > +					     info->len, READING_MODULE);
> > 
> > There was a lot of push back on calling security_kernel_read_file()
> > with a NULL file descriptor here.[1]  The result was defining a new
> > security hook - security_kernel_load_data - and enumeration -
> > LOADING_MODULE.  I would prefer calling the same pre and post security
> > hook.
> > 
> > Mimi
> > 
> > [1] http://kernsec.org/pipermail/linux-security-module-archive/2018-May/007110.html
> 
> Ah yes, thanks for the pointer to the discussion.
> 
> I think we have four cases then, for differing LSM hooks:
> 
> - no "file", no contents
> 	e.g. init_module() before copying user buffer
> 	security_kernel_load_data()
> - only a "file" available, no contents
> 	e.g. kernel_read_file() before actually reading anything
> 	security_kernel_read_file()
> - "file" and contents
> 	e.g. kernel_read_file() after reading
> 	security_kernel_post_read_file()
> - no "file" available, just the contents
> 	e.g. firmware platform fallback from EFI space (no "file")
> 	unimplemented!
> 
> If an LSM wants to be able to examine the contents of firmware, modules,
> kexec, etc, it needs either a "file" or the full contents.
> 
> The "file" methods all pass through the kernel_read_file()-family. The
> others happen via blobs coming from userspace or (more recently) the EFI
> universe.
> 
> So, if a NULL file is unreasonable, we need, perhaps,
> security_kernel_post_load_data()
> 
> ?

Agreed.

Mimi
diff mbox series

Patch

diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..af9679f8e5c6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2980,7 +2980,12 @@  static int copy_module_from_user(const void __user *umod, unsigned long len,
 		return -EFAULT;
 	}
 
-	return 0;
+	err = security_kernel_post_read_file(NULL, (char *)info->hdr,
+					     info->len, READING_MODULE);
+	if (err)
+		vfree(info->hdr);
+
+	return err;
 }
 
 static void free_copy(struct load_info *info)