On 5/22/25 00:03, Serge E. Hallyn wrote:
> On Wed, May 21, 2025 at 04:01:05PM +0200, Simon THOBY wrote:
>> Introduce a new hook to allow LSMs to decide whether to block the load
>> of a kernel module.
>>
>> Two hooks already exist:
>> - kernel_module_request is called when the kernel itself (not userspace)
>> request the load of a module, e.g. because a device was detected.
>> - security_kernel_load_data(LOADING_MODULE) is called when userspace calls
>> init_module/finit_module, but lack information about the module because
>> its headers have not been loaded into kernel space, let alone parsed.
>> This may not be sufficient for some LSMs.
>>
>> This new hook is similar to security_kernel_load_data(LOADING_MODULE),
>> but called after the module signature and header are verified, and only
>> takes the module name for now.
>>
>> Signed-off-by: Simon THOBY <git@xxxxxxxxxxxxx>
>> ---
>> include/linux/lsm_hook_defs.h | 1 +
>> include/linux/module.h | 1 +
>> include/linux/security.h | 6 ++++++
>> kernel/module/main.c | 4 ++++
>> security/security.c | 14 ++++++++++++++
>> 5 files changed, 26 insertions(+)
>>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index bf3bbac4e02a..51c5212d8bb6 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -223,6 +223,7 @@ LSM_HOOK(void, LSM_RET_VOID, cred_getlsmprop, const struct cred *c,
>> LSM_HOOK(int, 0, kernel_act_as, struct cred *new, u32 secid)
>> LSM_HOOK(int, 0, kernel_create_files_as, struct cred *new, struct inode *inode)
>> LSM_HOOK(int, 0, kernel_module_request, char *kmod_name)
>> +LSM_HOOK(int, 0, kernel_module_load, const char *kmod_name)
>> LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
>> LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
>> enum kernel_load_data_id id, char *description)
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 8050f77c3b64..b6b8d6f7f599 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -39,6 +39,7 @@ struct modversion_info {
>> char name[MODULE_NAME_LEN];
>> };
>>
>> +struct load_info;
>> struct module;
>> struct exception_table_entry;
>>
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index cc9b54d95d22..e175b2cc8caf 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -498,6 +498,7 @@ void security_cred_getlsmprop(const struct cred *c, struct lsm_prop *prop);
>> int security_kernel_act_as(struct cred *new, u32 secid);
>> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
>> int security_kernel_module_request(char *kmod_name);
>> +int security_kernel_module_load(const char *kmod_name);
>> int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
>> int security_kernel_post_load_data(char *buf, loff_t size,
>> enum kernel_load_data_id id,
>> @@ -1255,6 +1256,11 @@ static inline int security_kernel_module_request(char *kmod_name)
>> return 0;
>> }
>>
>> +static inline int security_kernel_module_load(const char *kmod_name)
>> +{
>> + return 0;
>> +}
>> +
>> static inline int security_kernel_load_data(enum kernel_load_data_id id, bool contents)
>> {
>> return 0;
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index a2859dc3eea6..12a1a5f4d823 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -3228,6 +3228,10 @@ static int early_mod_check(struct load_info *info, int flags)
>> return -EPERM;
>> }
>>
>> + err = security_kernel_module_load(info->name);
>
> Would it be more useful to pass in the whole info struct?
>
I thought about that, but was afraid the LSM hook is still called very early in
the boot process. I though the 'struct load_info' was only partially populated,
but upon further checking, you're right, and most fields of the structure were
already setup by the time the hook is called:
- len, hdr in the copy_module_from_user function
- sig_ok in module_sig_check
- sechdrs, secstrings, index, strtab and name in elf_validity_cache_copy
So I could definitely pass in the info struct instead.
On that note, I wonder if I should move 'struct load_info' out of kernel/module/internal.h,
because I'm fairly certain we don't want to have linux/security.h depending on an internal
header file from the module subsystem.
<snip>
