Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
From: Eric W. Biederman
Date: Tue May 29 2018 - 22:26:32 EST
Paul Moore <paul@xxxxxxxxxxxxxx> writes:
> 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.
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