Re: [RFC PATCH 1/9] LSM: Introduce a new hook: security_kernel_module_load

[Simon Thoby <git@nightmared.fr>]

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@nightmared.fr>
>> ---
>>  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>