diff mbox series

[2/3] security: add symbol namespace for reading file data

Message ID 20200513152108.25669-3-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series fs: reduce export usage of kerne_read*() calls | expand

Commit Message

Luis Chamberlain May 13, 2020, 3:21 p.m. UTC
Certain symbols are not meant to be used by everybody, the security
helpers for reading files directly is one such case. Use a symbol
namespace for them.

This will prevent abuse of use of these symbols in places they were
not inteded to be used, and provides an easy way to audit where these
types of operations happen as a whole.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 1 +
 fs/exec.c                               | 2 ++
 kernel/kexec.c                          | 2 ++
 kernel/module.c                         | 2 ++
 security/security.c                     | 6 +++---
 5 files changed, 10 insertions(+), 3 deletions(-)

Comments

Eric W. Biederman May 13, 2020, 3:40 p.m. UTC | #1
Luis Chamberlain <mcgrof@kernel.org> writes:

> Certain symbols are not meant to be used by everybody, the security
> helpers for reading files directly is one such case. Use a symbol
> namespace for them.
>
> This will prevent abuse of use of these symbols in places they were
> not inteded to be used, and provides an easy way to audit where these
> types of operations happen as a whole.

Why not just remove the ability for the firmware loader to be a module?

Is there some important use case that requires the firmware loader
to be a module?

We already compile the code in by default.  So it is probably just
easier to remove the modular support all together.  Which would allow
the export of the security hooks to be removed as well.

Eric


> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  drivers/base/firmware_loader/fallback.c | 1 +
>  fs/exec.c                               | 2 ++
>  kernel/kexec.c                          | 2 ++
>  kernel/module.c                         | 2 ++
>  security/security.c                     | 6 +++---
>  5 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index d9ac7296205e..b088886dafda 100644
> --- a/drivers/base/firmware_loader/fallback.c
> +++ b/drivers/base/firmware_loader/fallback.c
> @@ -19,6 +19,7 @@
>   */
>  
>  MODULE_IMPORT_NS(FIRMWARE_LOADER_PRIVATE);
> +MODULE_IMPORT_NS(SECURITY_READ);
>  
>  extern struct firmware_fallback_config fw_fallback_config;
>  
> diff --git a/fs/exec.c b/fs/exec.c
> index 9791b9eef9ce..30bd800ab1d6 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -72,6 +72,8 @@
>  
>  #include <trace/events/sched.h>
>  
> +MODULE_IMPORT_NS(SECURITY_READ);
> +
>  int suid_dumpable = 0;
>  
>  static LIST_HEAD(formats);
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index f977786fe498..8d572b41a157 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -19,6 +19,8 @@
>  
>  #include "kexec_internal.h"
>  
> +MODULE_IMPORT_NS(SECURITY_READ);
> +
>  static int copy_user_segment_list(struct kimage *image,
>  				  unsigned long nr_segments,
>  				  struct kexec_segment __user *segments)
> diff --git a/kernel/module.c b/kernel/module.c
> index 80faaf2116dd..8973a463712e 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -59,6 +59,8 @@
>  #include <uapi/linux/module.h>
>  #include "module-internal.h"
>  
> +MODULE_IMPORT_NS(SECURITY_READ);
> +
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/module.h>
>  
> diff --git a/security/security.c b/security/security.c
> index 8ae66e4c370f..bdbd1fc5105a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1654,7 +1654,7 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
>  		return ret;
>  	return ima_read_file(file, id);
>  }
> -EXPORT_SYMBOL_GPL(security_kernel_read_file);
> +EXPORT_SYMBOL_NS_GPL(security_kernel_read_file, SECURITY_READ);
>  
>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>  				   enum kernel_read_file_id id)
> @@ -1666,7 +1666,7 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>  		return ret;
>  	return ima_post_read_file(file, buf, size, id);
>  }
> -EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
> +EXPORT_SYMBOL_NS_GPL(security_kernel_post_read_file, SECURITY_READ);
>  
>  int security_kernel_load_data(enum kernel_load_data_id id)
>  {
> @@ -1677,7 +1677,7 @@ int security_kernel_load_data(enum kernel_load_data_id id)
>  		return ret;
>  	return ima_load_data(id);
>  }
> -EXPORT_SYMBOL_GPL(security_kernel_load_data);
> +EXPORT_SYMBOL_NS_GPL(security_kernel_load_data, SECURITY_READ);
>  
>  int security_task_fix_setuid(struct cred *new, const struct cred *old,
>  			     int flags)
Greg KH May 13, 2020, 4:09 p.m. UTC | #2
On Wed, May 13, 2020 at 10:40:31AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain <mcgrof@kernel.org> writes:
> 
> > Certain symbols are not meant to be used by everybody, the security
> > helpers for reading files directly is one such case. Use a symbol
> > namespace for them.
> >
> > This will prevent abuse of use of these symbols in places they were
> > not inteded to be used, and provides an easy way to audit where these
> > types of operations happen as a whole.
> 
> Why not just remove the ability for the firmware loader to be a module?

I agree, it's been a mess of build options to try to keep alive over
time.

> Is there some important use case that requires the firmware loader
> to be a module?

I don't think so anymore.

> We already compile the code in by default.  So it is probably just
> easier to remove the modular support all together.  Which would allow
> the export of the security hooks to be removed as well.

Agreed.

thanks,

greg k-h
Luis Chamberlain May 13, 2020, 4:16 p.m. UTC | #3
On Wed, May 13, 2020 at 10:40:31AM -0500, Eric W. Biederman wrote:
> Luis Chamberlain <mcgrof@kernel.org> writes:
> 
> > Certain symbols are not meant to be used by everybody, the security
> > helpers for reading files directly is one such case. Use a symbol
> > namespace for them.
> >
> > This will prevent abuse of use of these symbols in places they were
> > not inteded to be used, and provides an easy way to audit where these
> > types of operations happen as a whole.
> 
> Why not just remove the ability for the firmware loader to be a module?
> 
> Is there some important use case that requires the firmware loader
> to be a module?
> 
> We already compile the code in by default.  So it is probably just
> easier to remove the modular support all together.  Which would allow
> the export of the security hooks to be removed as well.

Yeah, that's a better solution. The only constaint I am aware of is
we *cannot* change the name of the module from firmware_class since the
old fallback sysfs loader depends on the module name. So, so long as we
take care with that on built-in and document this very well, I think
we should be good.

I checked the commit logs and this was tristate since the code was added
upstream, so I cannot see any good reason it was enabled as modular.

Speaking with a *backports experience* hat on, we did have a use case
to use a module for it in case a new feature was added upstream which
was not present on older kernels. However I think that using a separate
symbol prefix would help with that.

Would any Android stakeholders / small / embedded folks whave any issue
with this?

  Luis
Greg KH May 13, 2020, 4:26 p.m. UTC | #4
On Wed, May 13, 2020 at 04:16:22PM +0000, Luis Chamberlain wrote:
> On Wed, May 13, 2020 at 10:40:31AM -0500, Eric W. Biederman wrote:
> > Luis Chamberlain <mcgrof@kernel.org> writes:
> > 
> > > Certain symbols are not meant to be used by everybody, the security
> > > helpers for reading files directly is one such case. Use a symbol
> > > namespace for them.
> > >
> > > This will prevent abuse of use of these symbols in places they were
> > > not inteded to be used, and provides an easy way to audit where these
> > > types of operations happen as a whole.
> > 
> > Why not just remove the ability for the firmware loader to be a module?
> > 
> > Is there some important use case that requires the firmware loader
> > to be a module?
> > 
> > We already compile the code in by default.  So it is probably just
> > easier to remove the modular support all together.  Which would allow
> > the export of the security hooks to be removed as well.
> 
> Yeah, that's a better solution. The only constaint I am aware of is
> we *cannot* change the name of the module from firmware_class since the
> old fallback sysfs loader depends on the module name. So, so long as we
> take care with that on built-in and document this very well, I think
> we should be good.
> 
> I checked the commit logs and this was tristate since the code was added
> upstream, so I cannot see any good reason it was enabled as modular.
> 
> Speaking with a *backports experience* hat on, we did have a use case
> to use a module for it in case a new feature was added upstream which
> was not present on older kernels. However I think that using a separate
> symbol prefix would help with that.
> 
> Would any Android stakeholders / small / embedded folks whave any issue
> with this?

Android has build in the firmware loading logic for a while now.  Well,
always, they have not had kernel modules for many years.  That is
changing but this logic is not getting moved to a kernel module as that
would just be silly :)

thanks,

greg k-h
Josh Triplett May 13, 2020, 6:07 p.m. UTC | #5
On Wed, May 13, 2020 at 04:16:22PM +0000, Luis Chamberlain wrote:
> On Wed, May 13, 2020 at 10:40:31AM -0500, Eric W. Biederman wrote:
> > Luis Chamberlain <mcgrof@kernel.org> writes:
> > 
> > > Certain symbols are not meant to be used by everybody, the security
> > > helpers for reading files directly is one such case. Use a symbol
> > > namespace for them.
> > >
> > > This will prevent abuse of use of these symbols in places they were
> > > not inteded to be used, and provides an easy way to audit where these
> > > types of operations happen as a whole.
> > 
> > Why not just remove the ability for the firmware loader to be a module?
> > 
> > Is there some important use case that requires the firmware loader
> > to be a module?
> > 
> > We already compile the code in by default.  So it is probably just
> > easier to remove the modular support all together.  Which would allow
> > the export of the security hooks to be removed as well.
> 
> Yeah, that's a better solution. The only constaint I am aware of is
> we *cannot* change the name of the module from firmware_class since the
> old fallback sysfs loader depends on the module name. So, so long as we
> take care with that on built-in and document this very well, I think
> we should be good.
> 
> I checked the commit logs and this was tristate since the code was added
> upstream, so I cannot see any good reason it was enabled as modular.
> 
> Speaking with a *backports experience* hat on, we did have a use case
> to use a module for it in case a new feature was added upstream which
> was not present on older kernels. However I think that using a separate
> symbol prefix would help with that.
> 
> Would any Android stakeholders / small / embedded folks whave any issue
> with this?

As long as you can still *completely* compile out firmware loading, I
don't think there's a huge use case for making it modular. y/n seems
fine.
diff mbox series

Patch

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index d9ac7296205e..b088886dafda 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -19,6 +19,7 @@ 
  */
 
 MODULE_IMPORT_NS(FIRMWARE_LOADER_PRIVATE);
+MODULE_IMPORT_NS(SECURITY_READ);
 
 extern struct firmware_fallback_config fw_fallback_config;
 
diff --git a/fs/exec.c b/fs/exec.c
index 9791b9eef9ce..30bd800ab1d6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -72,6 +72,8 @@ 
 
 #include <trace/events/sched.h>
 
+MODULE_IMPORT_NS(SECURITY_READ);
+
 int suid_dumpable = 0;
 
 static LIST_HEAD(formats);
diff --git a/kernel/kexec.c b/kernel/kexec.c
index f977786fe498..8d572b41a157 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -19,6 +19,8 @@ 
 
 #include "kexec_internal.h"
 
+MODULE_IMPORT_NS(SECURITY_READ);
+
 static int copy_user_segment_list(struct kimage *image,
 				  unsigned long nr_segments,
 				  struct kexec_segment __user *segments)
diff --git a/kernel/module.c b/kernel/module.c
index 80faaf2116dd..8973a463712e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -59,6 +59,8 @@ 
 #include <uapi/linux/module.h>
 #include "module-internal.h"
 
+MODULE_IMPORT_NS(SECURITY_READ);
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
 
diff --git a/security/security.c b/security/security.c
index 8ae66e4c370f..bdbd1fc5105a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1654,7 +1654,7 @@  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
 		return ret;
 	return ima_read_file(file, id);
 }
-EXPORT_SYMBOL_GPL(security_kernel_read_file);
+EXPORT_SYMBOL_NS_GPL(security_kernel_read_file, SECURITY_READ);
 
 int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 				   enum kernel_read_file_id id)
@@ -1666,7 +1666,7 @@  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 		return ret;
 	return ima_post_read_file(file, buf, size, id);
 }
-EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
+EXPORT_SYMBOL_NS_GPL(security_kernel_post_read_file, SECURITY_READ);
 
 int security_kernel_load_data(enum kernel_load_data_id id)
 {
@@ -1677,7 +1677,7 @@  int security_kernel_load_data(enum kernel_load_data_id id)
 		return ret;
 	return ima_load_data(id);
 }
-EXPORT_SYMBOL_GPL(security_kernel_load_data);
+EXPORT_SYMBOL_NS_GPL(security_kernel_load_data, SECURITY_READ);
 
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
 			     int flags)