| Message ID | 1527616920-5415-9-git-send-email-zohar@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> > Cc: Jeff Vander Stoep <jeffv@google.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Casey Schaufler <casey@schaufler-ca.com> > --- > 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 >
On Tue, 2018-05-29 at 18:39 -0400, Paul Moore wrote: [...] > > @@ -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. There's some disagreement as to whether we really need two LSM hooks. This sounds like you would prefer a single LSM hook, not the two that this patch set introduces. We need to come to some consensus. (Comments appreciated in 0/8.) Mimi > > > 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 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> > > Cc: Jeff Vander Stoep <jeffv@google.com> > > Cc: Paul Moore <paul@paul-moore.com> > > Cc: Casey Schaufler <casey@schaufler-ca.com> > > --- > > 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 > > > -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Paul Moore <paul@paul-moore.com> writes: > On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> >> Cc: Jeff Vander Stoep <jeffv@google.com> >> Cc: Paul Moore <paul@paul-moore.com> >> Cc: Casey Schaufler <casey@schaufler-ca.com> >> --- >> 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. There are a couple of fundamental and strong differences here. selinux_kernel_load_data only has the current_sid to work with. selinux_module_data_from_file is all about the logic of how to get fsec or isec from the file and from the inode. For selinux and for every other lsm that uses the hooks that difference of whether or not you have a file leads to different logic and different code. There is no meaningful sharing between the two cases. In selinux all of the meaningful sharing happens with calls to avc_has_perm(... SYSTEM__MODULE_LOAD, ...); So as far as I can see talking about duplication is unfounded there is none. > 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. Except even today the actual logic is not shared in a single function. The only thing that happens in a single function is a switch statement that calls different functions. So what is the point of having a ``common'' function? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 29, 2018 at 7:14 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > On Tue, 2018-05-29 at 18:39 -0400, Paul Moore wrote: > [...] >> > @@ -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. > > There's some disagreement as to whether we really need two LSM hooks. > This sounds like you would prefer a single LSM hook, not the two that > this patch set introduces. > > We need to come to some consensus. (Comments appreciated in 0/8.) My comments were intentionally made on the SELinux specific code and not the LSM generic code/hooks. As the LSM hook code must not make any assumptions about the underlying LSM implementations, it may make sense to have two different hooks. However as far as the SELinux code is concerned I would rather keep the access controls in one function as mentioned above. From a purely selfish SELinux perspective I would prefer a single hook, but if others feel two hooks are better, that's fine with me too. >> > 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
On Tue, May 29, 2018 at 10:25 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Paul Moore <paul@paul-moore.com> writes: >> On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> >>> Cc: Jeff Vander Stoep <jeffv@google.com> >>> Cc: Paul Moore <paul@paul-moore.com> >>> Cc: Casey Schaufler <casey@schaufler-ca.com> >>> --- >>> 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/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. > > There are a couple of fundamental and strong differences here. > > selinux_kernel_load_data only has the current_sid to work with. > > selinux_module_data_from_file is all about the logic of how > to get fsec or isec from the file and from the inode. > > For selinux and for every other lsm that uses the hooks that difference > of whether or not you have a file leads to different logic and different > code. There is no meaningful sharing between the two cases. > > In selinux all of the meaningful sharing happens with calls to > avc_has_perm(... SYSTEM__MODULE_LOAD, ...); > > So as far as I can see talking about duplication is unfounded there is > none. The duplication is the system:module_load access check (look at the avc_has_perm() calls in selinux_kernel_module_from_file(). I believe there is a readability/maintainability advantage to keeping them in one function, and I would like it to stay that way. If these particular LSM hook(s) ever became performance critical (e.g. many invocations a second) then I could see the value in separating them out to eliminating some conditionals/branching, but as far as I can tell that is not a problem at present. I'm guessing your concern is more about the LSM hooks themselves and not the individual implementations, in which case please see my last response to Mimi. I'm not opposed to two separate LSM hooks, I just want to avoid moving the system:module_load checks out of selinux_kernel_module_from_file(); which is independent of the number of LSM hooks.
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; +} + 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),
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@linux.vnet.ibm.com> Cc: Jeff Vander Stoep <jeffv@google.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Casey Schaufler <casey@schaufler-ca.com> --- kernel/module.c | 2 +- security/integrity/ima/ima_main.c | 24 ++++++++++-------------- security/selinux/hooks.c | 26 ++++++++++++++++++++------ 3 files changed, 31 insertions(+), 21 deletions(-)