Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
From: Mimi Zohar
Date: Tue May 29 2018 - 19:25:21 EST
Hi Kees,
Missing from this patch are the loadpin changes. ÂBefore including
them in the next version of this patch, do you prefer separating the
init_module from the finit_module support in loadpin_read_file() or
keeping it as one function, like Paul for SELinux?
Mimi
On Tue, 2018-05-29 at 18:39 -0400, Paul Moore wrote:
> On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> > Both the init_module and finit_module syscalls call either directly
> > or indirectly the security_kernel_read_file LSM hook. This patch
> > replaces the direct call in init_module with a call to the new
> > security_kernel_load_data hook and makes the corresponding changes in
> > SELinux and IMA.
> >
> > Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx>
> > Cc: Jeff Vander Stoep <jeffv@xxxxxxxxxx>
> > Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
> > Cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > ---
> > kernel/module.c | 2 +-
> > security/integrity/ima/ima_main.c | 24 ++++++++++--------------
> > security/selinux/hooks.c | 26 ++++++++++++++++++++------
> > 3 files changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index ce8066b88178..b97c642b5b4d 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> > if (info->len < sizeof(*(info->hdr)))
> > return -ENOEXEC;
> >
> > - err = security_kernel_read_file(NULL, READING_MODULE);
> > + err = security_kernel_load_data(LOADING_MODULE);
> > if (err)
> > return err;
> >
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 3dae605a1604..0ff1d8152f6e 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -441,17 +441,6 @@ static int read_idmap[READING_MAX_ID] = {
> > */
> > int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> > {
> > - bool sig_enforce = is_module_sig_enforced();
> > -
> > - if (!file && read_id == READING_MODULE) {
> > - if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> > - (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > - pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> > - return -EACCES; /* INTEGRITY_UNKNOWN */
> > - }
> > - return 0; /* We rely on module signature checking */
> > - }
> > -
> > if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
> > if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> > (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > @@ -490,9 +479,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > return 0;
> > }
> >
> > - if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
> > - return 0;
> > -
> > /* permit signed certs */
> > if (!file && read_id == READING_X509_CERTIFICATE)
> > return 0;
> > @@ -521,6 +507,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> > */
> > int ima_load_data(enum kernel_load_data_id id)
> > {
> > + bool sig_enforce;
> > +
> > if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
> > return 0;
> >
> > @@ -536,6 +524,14 @@ int ima_load_data(enum kernel_load_data_id id)
> > pr_err("Prevent firmware sysfs fallback loading.\n");
> > return -EACCES; /* INTEGRITY_UNKNOWN */
> > }
> > + break;
> > + case LOADING_MODULE:
> > + sig_enforce = is_module_sig_enforced();
> > +
> > + if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
> > + pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> > + return -EACCES; /* INTEGRITY_UNKNOWN */
> > + }
> > default:
> > break;
> > }
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 02ebd1585eaf..e02186470fc5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4018,12 +4018,6 @@ static int selinux_kernel_module_from_file(struct file *file)
> > u32 sid = current_sid();
> > int rc;
> >
> > - /* init_module */
> > - if (file == NULL)
> > - return avc_has_perm(&selinux_state,
> > - sid, sid, SECCLASS_SYSTEM,
> > - SYSTEM__MODULE_LOAD, NULL);
> > -
> > /* finit_module */
> >
> > ad.type = LSM_AUDIT_DATA_FILE;
> > @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
> > SYSTEM__MODULE_LOAD, &ad);
> > }
> >
> > +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> > +{
> > + u32 sid;
> > + int rc = 0;
> > +
> > + switch (id) {
> > + case LOADING_MODULE:
> > + sid = current_sid();
> > +
> > + /* init_module */
> > + return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
> > + SYSTEM__MODULE_LOAD, NULL);
> > + default:
> > + break;
> > + }
> > +
> > + return rc;
> > +}
>
> I'm not a fan of the duplication here. If we must have a new LSM hook
> for this, can we at least have it call
> selinux_kernel_module_from_file() so we have all the kernel module
> loading logic/controls in one function? Yes, I understand there are
> differences between init_module() and finit_module() but I like
> handling them both in one function as we do today.
>
> > static int selinux_kernel_read_file(struct file *file,
> > enum kernel_read_file_id id)
> > {
> > @@ -6950,6 +6963,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> > LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
> > LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
> > LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> > + LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
> > LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
> > LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
> > LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> > --
> > 2.7.5
> >
>