diff mbox series

[v1] audit,module: restore audit logging in load failure case

Message ID 999cdd694f951acd2f4ad665fe7ab97d0834e162.1729717542.git.rgb@redhat.com (mailing list archive)
State Handled Elsewhere
Headers show
Series [v1] audit,module: restore audit logging in load failure case | expand

Checks

Context Check Description
mcgrof/vmtest-main-PR fail merge-conflict
mcgrof/vmtest-modules-next-PR fail PR summary
mcgrof/vmtest-modules-next-VM_Test-0 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-3 success Logs for cleanup
mcgrof/vmtest-modules-next-VM_Test-1 success Logs for Run CI tests
mcgrof/vmtest-modules-next-VM_Test-2 success Logs for cleanup
mcgrof/vmtest-modules-next-VM_Test-4 fail Logs for setup / Setup kdevops environment
mcgrof/vmtest-modules-next-VM_Test-5 fail Logs for setup / Setup kdevops environment

Commit Message

Richard Guy Briggs Oct. 23, 2024, 9:13 p.m. UTC 
The move of the module sanity check to earlier skipped the audit logging
call in the case of failure and to a place where the previously used
context is unavailable.

Add an audit logging call for the module loading failure case and get
the module name when possible.

Link: https://issues.redhat.com/browse/RHEL-52839
Fixes: 02da2cbab452 ("module: move check_modinfo() early to early_mod_check()")
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/module/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

kernel test robot Oct. 24, 2024, 5:58 p.m. UTC | #1 
Hi Richard,

kernel test robot noticed the following build errors:

[auto build test ERROR on mcgrof/modules-next]
[also build test ERROR on linus/master v6.12-rc4 next-20241024]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/audit-module-restore-audit-logging-in-load-failure-case/20241024-051515
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/999cdd694f951acd2f4ad665fe7ab97d0834e162.1729717542.git.rgb%40redhat.com
patch subject: [PATCH v1] audit,module: restore audit logging in load failure case
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20241025/202410250152.vcJ5tyVZ-lkp@intel.com/config)
compiler: clang version 19.1.2 (https://github.com/llvm/llvm-project 7ba7d8e2f7b6445b60679da826210cdde29eaf8b)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410250152.vcJ5tyVZ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410250152.vcJ5tyVZ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/module/main.c:14:
   In file included from include/linux/trace_events.h:6:
   In file included from include/linux/ring_buffer.h:5:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> kernel/module/main.c:3336:25: error: passing 'const char *' to parameter of type 'char *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
    3336 |                 audit_log_kern_module(info->name ? info->name : "(unavailable)");
         |                                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/audit.h:522:48: note: passing argument to parameter 'name' here
     522 | static inline void audit_log_kern_module(char *name)
         |                                                ^
   4 warnings and 1 error generated.


vim +3336 kernel/module/main.c

  3124	
  3125	/*
  3126	 * Allocate and load the module: note that size of section 0 is always
  3127	 * zero, and we rely on this for optional sections.
  3128	 */
  3129	static int load_module(struct load_info *info, const char __user *uargs,
  3130			       int flags)
  3131	{
  3132		struct module *mod;
  3133		bool module_allocated = false;
  3134		long err = 0;
  3135		char *after_dashes;
  3136	
  3137		/*
  3138		 * Do the signature check (if any) first. All that
  3139		 * the signature check needs is info->len, it does
  3140		 * not need any of the section info. That can be
  3141		 * set up later. This will minimize the chances
  3142		 * of a corrupt module causing problems before
  3143		 * we even get to the signature check.
  3144		 *
  3145		 * The check will also adjust info->len by stripping
  3146		 * off the sig length at the end of the module, making
  3147		 * checks against info->len more correct.
  3148		 */
  3149		err = module_sig_check(info, flags);
  3150		if (err)
  3151			goto free_copy;
  3152	
  3153		/*
  3154		 * Do basic sanity checks against the ELF header and
  3155		 * sections. Cache useful sections and set the
  3156		 * info->mod to the userspace passed struct module.
  3157		 */
  3158		err = elf_validity_cache_copy(info, flags);
  3159		if (err)
  3160			goto free_copy;
  3161	
  3162		err = early_mod_check(info, flags);
  3163		if (err)
  3164			goto free_copy;
  3165	
  3166		/* Figure out module layout, and allocate all the memory. */
  3167		mod = layout_and_allocate(info, flags);
  3168		if (IS_ERR(mod)) {
  3169			err = PTR_ERR(mod);
  3170			goto free_copy;
  3171		}
  3172	
  3173		module_allocated = true;
  3174	
  3175		audit_log_kern_module(mod->name);
  3176	
  3177		/* Reserve our place in the list. */
  3178		err = add_unformed_module(mod);
  3179		if (err)
  3180			goto free_module;
  3181	
  3182		/*
  3183		 * We are tainting your kernel if your module gets into
  3184		 * the modules linked list somehow.
  3185		 */
  3186		module_augment_kernel_taints(mod, info);
  3187	
  3188		/* To avoid stressing percpu allocator, do this once we're unique. */
  3189		err = percpu_modalloc(mod, info);
  3190		if (err)
  3191			goto unlink_mod;
  3192	
  3193		/* Now module is in final location, initialize linked lists, etc. */
  3194		err = module_unload_init(mod);
  3195		if (err)
  3196			goto unlink_mod;
  3197	
  3198		init_param_lock(mod);
  3199	
  3200		/*
  3201		 * Now we've got everything in the final locations, we can
  3202		 * find optional sections.
  3203		 */
  3204		err = find_module_sections(mod, info);
  3205		if (err)
  3206			goto free_unload;
  3207	
  3208		err = check_export_symbol_versions(mod);
  3209		if (err)
  3210			goto free_unload;
  3211	
  3212		/* Set up MODINFO_ATTR fields */
  3213		setup_modinfo(mod, info);
  3214	
  3215		/* Fix up syms, so that st_value is a pointer to location. */
  3216		err = simplify_symbols(mod, info);
  3217		if (err < 0)
  3218			goto free_modinfo;
  3219	
  3220		err = apply_relocations(mod, info);
  3221		if (err < 0)
  3222			goto free_modinfo;
  3223	
  3224		err = post_relocation(mod, info);
  3225		if (err < 0)
  3226			goto free_modinfo;
  3227	
  3228		flush_module_icache(mod);
  3229	
  3230		/* Now copy in args */
  3231		mod->args = strndup_user(uargs, ~0UL >> 1);
  3232		if (IS_ERR(mod->args)) {
  3233			err = PTR_ERR(mod->args);
  3234			goto free_arch_cleanup;
  3235		}
  3236	
  3237		init_build_id(mod, info);
  3238	
  3239		/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
  3240		ftrace_module_init(mod);
  3241	
  3242		/* Finally it's fully formed, ready to start executing. */
  3243		err = complete_formation(mod, info);
  3244		if (err)
  3245			goto ddebug_cleanup;
  3246	
  3247		err = prepare_coming_module(mod);
  3248		if (err)
  3249			goto bug_cleanup;
  3250	
  3251		mod->async_probe_requested = async_probe;
  3252	
  3253		/* Module is ready to execute: parsing args may do that. */
  3254		after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
  3255					  -32768, 32767, mod,
  3256					  unknown_module_param_cb);
  3257		if (IS_ERR(after_dashes)) {
  3258			err = PTR_ERR(after_dashes);
  3259			goto coming_cleanup;
  3260		} else if (after_dashes) {
  3261			pr_warn("%s: parameters '%s' after `--' ignored\n",
  3262			       mod->name, after_dashes);
  3263		}
  3264	
  3265		/* Link in to sysfs. */
  3266		err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
  3267		if (err < 0)
  3268			goto coming_cleanup;
  3269	
  3270		if (is_livepatch_module(mod)) {
  3271			err = copy_module_elf(mod, info);
  3272			if (err < 0)
  3273				goto sysfs_cleanup;
  3274		}
  3275	
  3276		/* Get rid of temporary copy. */
  3277		free_copy(info, flags);
  3278	
  3279		codetag_load_module(mod);
  3280	
  3281		/* Done! */
  3282		trace_module_load(mod);
  3283	
  3284		return do_init_module(mod);
  3285	
  3286	 sysfs_cleanup:
  3287		mod_sysfs_teardown(mod);
  3288	 coming_cleanup:
  3289		mod->state = MODULE_STATE_GOING;
  3290		destroy_params(mod->kp, mod->num_kp);
  3291		blocking_notifier_call_chain(&module_notify_list,
  3292					     MODULE_STATE_GOING, mod);
  3293		klp_module_going(mod);
  3294	 bug_cleanup:
  3295		mod->state = MODULE_STATE_GOING;
  3296		/* module_bug_cleanup needs module_mutex protection */
  3297		mutex_lock(&module_mutex);
  3298		module_bug_cleanup(mod);
  3299		mutex_unlock(&module_mutex);
  3300	
  3301	 ddebug_cleanup:
  3302		ftrace_release_mod(mod);
  3303		synchronize_rcu();
  3304		kfree(mod->args);
  3305	 free_arch_cleanup:
  3306		module_arch_cleanup(mod);
  3307	 free_modinfo:
  3308		free_modinfo(mod);
  3309	 free_unload:
  3310		module_unload_free(mod);
  3311	 unlink_mod:
  3312		mutex_lock(&module_mutex);
  3313		/* Unlink carefully: kallsyms could be walking list. */
  3314		list_del_rcu(&mod->list);
  3315		mod_tree_remove(mod);
  3316		wake_up_all(&module_wq);
  3317		/* Wait for RCU-sched synchronizing before releasing mod->list. */
  3318		synchronize_rcu();
  3319		mutex_unlock(&module_mutex);
  3320	 free_module:
  3321		mod_stat_bump_invalid(info, flags);
  3322		/* Free lock-classes; relies on the preceding sync_rcu() */
  3323		for_class_mod_mem_type(type, core_data) {
  3324			lockdep_free_key_range(mod->mem[type].base,
  3325					       mod->mem[type].size);
  3326		}
  3327	
  3328		module_deallocate(mod, info);
  3329	 free_copy:
  3330		/*
  3331		 * The info->len is always set. We distinguish between
  3332		 * failures once the proper module was allocated and
  3333		 * before that.
  3334		 */
  3335		if (!module_allocated) {
> 3336			audit_log_kern_module(info->name ? info->name : "(unavailable)");
  3337			mod_stat_bump_becoming(info, flags);
  3338		}
  3339		free_copy(info, flags);
  3340		return err;
  3341	}
  3342
Paul Moore Oct. 24, 2024, 8:41 p.m. UTC | #2 
On Oct 23, 2024 Richard Guy Briggs <rgb@redhat.com> wrote:
> 
> The move of the module sanity check to earlier skipped the audit logging
> call in the case of failure and to a place where the previously used
> context is unavailable.
> 
> Add an audit logging call for the module loading failure case and get
> the module name when possible.
> 
> Link: https://issues.redhat.com/browse/RHEL-52839
> Fixes: 02da2cbab452 ("module: move check_modinfo() early to early_mod_check()")
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/module/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 49b9bca9de12..1f482532ef66 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3057,8 +3057,10 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  	 * failures once the proper module was allocated and
>  	 * before that.
>  	 */
> -	if (!module_allocated)
> +	if (!module_allocated) {
> +		audit_log_kern_module(info->name ? info->name : "(unavailable)");
>  		mod_stat_bump_becoming(info, flags);
> +	}

We probably should move the existing audit_log_kern_module() to just
after the elf_validity_cache_copy() call as both info->name and
info->mod->name should be as valid as they are going to get at that
point.  If we do that then we only have two cases we need to worry about,
a failed module_sig_check() or a failed elf_validity_cache_copy(), and
in both cases we can use "(unavailable)" without having to check
info->name first.

However, assuming we move the audit_log_kern_module() call up a bit as
described above, I'm not sure there is much value in calling
audit_log_kern_module() with an "(unavailable)" module name in those
early two cases.  We know it's an attempted module load based on the
SYSCALL record, seeing an associated "(unavailable)" KERN_MODULE record
doesn't provide us with any more information than if we had simply
skipped the KERN_MODULE record.

Untested, but this is what I'm talking about:

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 0050ef288ab3..eaa10e3c7eca 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -417,7 +417,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm,
 extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_openat2_how(struct open_how *how);
-extern void __audit_log_kern_module(char *name);
+extern void __audit_log_kern_module(const char *name);
 extern void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar);
 extern void __audit_tk_injoffset(struct timespec64 offset);
 extern void __audit_ntp_log(const struct audit_ntp_data *ad);
@@ -519,7 +519,7 @@ static inline void audit_openat2_how(struct open_how *how)
                __audit_openat2_how(how);
 }
 
-static inline void audit_log_kern_module(char *name)
+static inline void audit_log_kern_module(const char *name)
 {
        if (!audit_dummy_context())
                __audit_log_kern_module(name);
@@ -677,7 +677,7 @@ static inline void audit_mmap_fd(int fd, int flags)
 static inline void audit_openat2_how(struct open_how *how)
 { }
 
-static inline void audit_log_kern_module(char *name)
+static inline void audit_log_kern_module(const char *name)
 {
 }
 
diff --git a/kernel/audit.h b/kernel/audit.h
index a60d2840559e..5156ecd35457 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -199,7 +199,7 @@ struct audit_context {
                        int                     argc;
                } execve;
                struct {
-                       char                    *name;
+                       const char              *name;
                } module;
                struct {
                        struct audit_ntp_data   ntp_data;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 0627e74585ce..f79eb3a5a789 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2870,7 +2870,7 @@ void __audit_openat2_how(struct open_how *how)
        context->type = AUDIT_OPENAT2;
 }
 
-void __audit_log_kern_module(char *name)
+void __audit_log_kern_module(const char *name)
 {
        struct audit_context *context = audit_context();
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 49b9bca9de12..3acb65073c53 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2884,6 +2884,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
        if (err)
                goto free_copy;
 
+       audit_log_kern_module(info->name);
+
        err = early_mod_check(info, flags);
        if (err)
                goto free_copy;
@@ -2897,8 +2899,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
        module_allocated = true;
 
-       audit_log_kern_module(mod->name);
-
        /* Reserve our place in the list. */
        err = add_unformed_module(mod);
        if (err)

--
paul-moore.com
kernel test robot Oct. 25, 2024, 7:38 a.m. UTC | #3 
Hi Richard,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mcgrof/modules-next]
[also build test WARNING on linus/master v6.12-rc4 next-20241024]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/audit-module-restore-audit-logging-in-load-failure-case/20241024-051515
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/999cdd694f951acd2f4ad665fe7ab97d0834e162.1729717542.git.rgb%40redhat.com
patch subject: [PATCH v1] audit,module: restore audit logging in load failure case
config: x86_64-randconfig-121-20241025 (https://download.01.org/0day-ci/archive/20241025/202410251446.xzMTe7Yk-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241025/202410251446.xzMTe7Yk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410251446.xzMTe7Yk-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> kernel/module/main.c:3336:50: sparse: sparse: incorrect type in argument 1 (different modifiers) @@     expected char *name @@     got char const * @@
   kernel/module/main.c:3336:50: sparse:     expected char *name
   kernel/module/main.c:3336:50: sparse:     got char const *

vim +3336 kernel/module/main.c

  3124	
  3125	/*
  3126	 * Allocate and load the module: note that size of section 0 is always
  3127	 * zero, and we rely on this for optional sections.
  3128	 */
  3129	static int load_module(struct load_info *info, const char __user *uargs,
  3130			       int flags)
  3131	{
  3132		struct module *mod;
  3133		bool module_allocated = false;
  3134		long err = 0;
  3135		char *after_dashes;
  3136	
  3137		/*
  3138		 * Do the signature check (if any) first. All that
  3139		 * the signature check needs is info->len, it does
  3140		 * not need any of the section info. That can be
  3141		 * set up later. This will minimize the chances
  3142		 * of a corrupt module causing problems before
  3143		 * we even get to the signature check.
  3144		 *
  3145		 * The check will also adjust info->len by stripping
  3146		 * off the sig length at the end of the module, making
  3147		 * checks against info->len more correct.
  3148		 */
  3149		err = module_sig_check(info, flags);
  3150		if (err)
  3151			goto free_copy;
  3152	
  3153		/*
  3154		 * Do basic sanity checks against the ELF header and
  3155		 * sections. Cache useful sections and set the
  3156		 * info->mod to the userspace passed struct module.
  3157		 */
  3158		err = elf_validity_cache_copy(info, flags);
  3159		if (err)
  3160			goto free_copy;
  3161	
  3162		err = early_mod_check(info, flags);
  3163		if (err)
  3164			goto free_copy;
  3165	
  3166		/* Figure out module layout, and allocate all the memory. */
  3167		mod = layout_and_allocate(info, flags);
  3168		if (IS_ERR(mod)) {
  3169			err = PTR_ERR(mod);
  3170			goto free_copy;
  3171		}
  3172	
  3173		module_allocated = true;
  3174	
  3175		audit_log_kern_module(mod->name);
  3176	
  3177		/* Reserve our place in the list. */
  3178		err = add_unformed_module(mod);
  3179		if (err)
  3180			goto free_module;
  3181	
  3182		/*
  3183		 * We are tainting your kernel if your module gets into
  3184		 * the modules linked list somehow.
  3185		 */
  3186		module_augment_kernel_taints(mod, info);
  3187	
  3188		/* To avoid stressing percpu allocator, do this once we're unique. */
  3189		err = percpu_modalloc(mod, info);
  3190		if (err)
  3191			goto unlink_mod;
  3192	
  3193		/* Now module is in final location, initialize linked lists, etc. */
  3194		err = module_unload_init(mod);
  3195		if (err)
  3196			goto unlink_mod;
  3197	
  3198		init_param_lock(mod);
  3199	
  3200		/*
  3201		 * Now we've got everything in the final locations, we can
  3202		 * find optional sections.
  3203		 */
  3204		err = find_module_sections(mod, info);
  3205		if (err)
  3206			goto free_unload;
  3207	
  3208		err = check_export_symbol_versions(mod);
  3209		if (err)
  3210			goto free_unload;
  3211	
  3212		/* Set up MODINFO_ATTR fields */
  3213		setup_modinfo(mod, info);
  3214	
  3215		/* Fix up syms, so that st_value is a pointer to location. */
  3216		err = simplify_symbols(mod, info);
  3217		if (err < 0)
  3218			goto free_modinfo;
  3219	
  3220		err = apply_relocations(mod, info);
  3221		if (err < 0)
  3222			goto free_modinfo;
  3223	
  3224		err = post_relocation(mod, info);
  3225		if (err < 0)
  3226			goto free_modinfo;
  3227	
  3228		flush_module_icache(mod);
  3229	
  3230		/* Now copy in args */
  3231		mod->args = strndup_user(uargs, ~0UL >> 1);
  3232		if (IS_ERR(mod->args)) {
  3233			err = PTR_ERR(mod->args);
  3234			goto free_arch_cleanup;
  3235		}
  3236	
  3237		init_build_id(mod, info);
  3238	
  3239		/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
  3240		ftrace_module_init(mod);
  3241	
  3242		/* Finally it's fully formed, ready to start executing. */
  3243		err = complete_formation(mod, info);
  3244		if (err)
  3245			goto ddebug_cleanup;
  3246	
  3247		err = prepare_coming_module(mod);
  3248		if (err)
  3249			goto bug_cleanup;
  3250	
  3251		mod->async_probe_requested = async_probe;
  3252	
  3253		/* Module is ready to execute: parsing args may do that. */
  3254		after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
  3255					  -32768, 32767, mod,
  3256					  unknown_module_param_cb);
  3257		if (IS_ERR(after_dashes)) {
  3258			err = PTR_ERR(after_dashes);
  3259			goto coming_cleanup;
  3260		} else if (after_dashes) {
  3261			pr_warn("%s: parameters '%s' after `--' ignored\n",
  3262			       mod->name, after_dashes);
  3263		}
  3264	
  3265		/* Link in to sysfs. */
  3266		err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
  3267		if (err < 0)
  3268			goto coming_cleanup;
  3269	
  3270		if (is_livepatch_module(mod)) {
  3271			err = copy_module_elf(mod, info);
  3272			if (err < 0)
  3273				goto sysfs_cleanup;
  3274		}
  3275	
  3276		/* Get rid of temporary copy. */
  3277		free_copy(info, flags);
  3278	
  3279		codetag_load_module(mod);
  3280	
  3281		/* Done! */
  3282		trace_module_load(mod);
  3283	
  3284		return do_init_module(mod);
  3285	
  3286	 sysfs_cleanup:
  3287		mod_sysfs_teardown(mod);
  3288	 coming_cleanup:
  3289		mod->state = MODULE_STATE_GOING;
  3290		destroy_params(mod->kp, mod->num_kp);
  3291		blocking_notifier_call_chain(&module_notify_list,
  3292					     MODULE_STATE_GOING, mod);
  3293		klp_module_going(mod);
  3294	 bug_cleanup:
  3295		mod->state = MODULE_STATE_GOING;
  3296		/* module_bug_cleanup needs module_mutex protection */
  3297		mutex_lock(&module_mutex);
  3298		module_bug_cleanup(mod);
  3299		mutex_unlock(&module_mutex);
  3300	
  3301	 ddebug_cleanup:
  3302		ftrace_release_mod(mod);
  3303		synchronize_rcu();
  3304		kfree(mod->args);
  3305	 free_arch_cleanup:
  3306		module_arch_cleanup(mod);
  3307	 free_modinfo:
  3308		free_modinfo(mod);
  3309	 free_unload:
  3310		module_unload_free(mod);
  3311	 unlink_mod:
  3312		mutex_lock(&module_mutex);
  3313		/* Unlink carefully: kallsyms could be walking list. */
  3314		list_del_rcu(&mod->list);
  3315		mod_tree_remove(mod);
  3316		wake_up_all(&module_wq);
  3317		/* Wait for RCU-sched synchronizing before releasing mod->list. */
  3318		synchronize_rcu();
  3319		mutex_unlock(&module_mutex);
  3320	 free_module:
  3321		mod_stat_bump_invalid(info, flags);
  3322		/* Free lock-classes; relies on the preceding sync_rcu() */
  3323		for_class_mod_mem_type(type, core_data) {
  3324			lockdep_free_key_range(mod->mem[type].base,
  3325					       mod->mem[type].size);
  3326		}
  3327	
  3328		module_deallocate(mod, info);
  3329	 free_copy:
  3330		/*
  3331		 * The info->len is always set. We distinguish between
  3332		 * failures once the proper module was allocated and
  3333		 * before that.
  3334		 */
  3335		if (!module_allocated) {
> 3336			audit_log_kern_module(info->name ? info->name : "(unavailable)");
  3337			mod_stat_bump_becoming(info, flags);
  3338		}
  3339		free_copy(info, flags);
  3340		return err;
  3341	}
  3342
kernel test robot Oct. 28, 2024, 5:55 a.m. UTC | #4 
Hi Richard,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mcgrof/modules-next]
[also build test WARNING on linus/master v6.12-rc5 next-20241025]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/audit-module-restore-audit-logging-in-load-failure-case/20241024-051515
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git modules-next
patch link:    https://lore.kernel.org/r/999cdd694f951acd2f4ad665fe7ab97d0834e162.1729717542.git.rgb%40redhat.com
patch subject: [PATCH v1] audit,module: restore audit logging in load failure case
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20241028/202410281314.56S1Nfqd-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241028/202410281314.56S1Nfqd-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410281314.56S1Nfqd-lkp@intel.com/

All warnings (new ones prefixed by >>):

   kernel/module/main.c: In function 'load_module':
>> kernel/module/main.c:3336:63: warning: passing argument 1 of 'audit_log_kern_module' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    3336 |                 audit_log_kern_module(info->name ? info->name : "(unavailable)");
         |                                       ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
   In file included from kernel/module/main.c:57:
   include/linux/audit.h:522:48: note: expected 'char *' but argument is of type 'const char *'
     522 | static inline void audit_log_kern_module(char *name)
         |                                          ~~~~~~^~~~


vim +3336 kernel/module/main.c

  3124	
  3125	/*
  3126	 * Allocate and load the module: note that size of section 0 is always
  3127	 * zero, and we rely on this for optional sections.
  3128	 */
  3129	static int load_module(struct load_info *info, const char __user *uargs,
  3130			       int flags)
  3131	{
  3132		struct module *mod;
  3133		bool module_allocated = false;
  3134		long err = 0;
  3135		char *after_dashes;
  3136	
  3137		/*
  3138		 * Do the signature check (if any) first. All that
  3139		 * the signature check needs is info->len, it does
  3140		 * not need any of the section info. That can be
  3141		 * set up later. This will minimize the chances
  3142		 * of a corrupt module causing problems before
  3143		 * we even get to the signature check.
  3144		 *
  3145		 * The check will also adjust info->len by stripping
  3146		 * off the sig length at the end of the module, making
  3147		 * checks against info->len more correct.
  3148		 */
  3149		err = module_sig_check(info, flags);
  3150		if (err)
  3151			goto free_copy;
  3152	
  3153		/*
  3154		 * Do basic sanity checks against the ELF header and
  3155		 * sections. Cache useful sections and set the
  3156		 * info->mod to the userspace passed struct module.
  3157		 */
  3158		err = elf_validity_cache_copy(info, flags);
  3159		if (err)
  3160			goto free_copy;
  3161	
  3162		err = early_mod_check(info, flags);
  3163		if (err)
  3164			goto free_copy;
  3165	
  3166		/* Figure out module layout, and allocate all the memory. */
  3167		mod = layout_and_allocate(info, flags);
  3168		if (IS_ERR(mod)) {
  3169			err = PTR_ERR(mod);
  3170			goto free_copy;
  3171		}
  3172	
  3173		module_allocated = true;
  3174	
  3175		audit_log_kern_module(mod->name);
  3176	
  3177		/* Reserve our place in the list. */
  3178		err = add_unformed_module(mod);
  3179		if (err)
  3180			goto free_module;
  3181	
  3182		/*
  3183		 * We are tainting your kernel if your module gets into
  3184		 * the modules linked list somehow.
  3185		 */
  3186		module_augment_kernel_taints(mod, info);
  3187	
  3188		/* To avoid stressing percpu allocator, do this once we're unique. */
  3189		err = percpu_modalloc(mod, info);
  3190		if (err)
  3191			goto unlink_mod;
  3192	
  3193		/* Now module is in final location, initialize linked lists, etc. */
  3194		err = module_unload_init(mod);
  3195		if (err)
  3196			goto unlink_mod;
  3197	
  3198		init_param_lock(mod);
  3199	
  3200		/*
  3201		 * Now we've got everything in the final locations, we can
  3202		 * find optional sections.
  3203		 */
  3204		err = find_module_sections(mod, info);
  3205		if (err)
  3206			goto free_unload;
  3207	
  3208		err = check_export_symbol_versions(mod);
  3209		if (err)
  3210			goto free_unload;
  3211	
  3212		/* Set up MODINFO_ATTR fields */
  3213		setup_modinfo(mod, info);
  3214	
  3215		/* Fix up syms, so that st_value is a pointer to location. */
  3216		err = simplify_symbols(mod, info);
  3217		if (err < 0)
  3218			goto free_modinfo;
  3219	
  3220		err = apply_relocations(mod, info);
  3221		if (err < 0)
  3222			goto free_modinfo;
  3223	
  3224		err = post_relocation(mod, info);
  3225		if (err < 0)
  3226			goto free_modinfo;
  3227	
  3228		flush_module_icache(mod);
  3229	
  3230		/* Now copy in args */
  3231		mod->args = strndup_user(uargs, ~0UL >> 1);
  3232		if (IS_ERR(mod->args)) {
  3233			err = PTR_ERR(mod->args);
  3234			goto free_arch_cleanup;
  3235		}
  3236	
  3237		init_build_id(mod, info);
  3238	
  3239		/* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
  3240		ftrace_module_init(mod);
  3241	
  3242		/* Finally it's fully formed, ready to start executing. */
  3243		err = complete_formation(mod, info);
  3244		if (err)
  3245			goto ddebug_cleanup;
  3246	
  3247		err = prepare_coming_module(mod);
  3248		if (err)
  3249			goto bug_cleanup;
  3250	
  3251		mod->async_probe_requested = async_probe;
  3252	
  3253		/* Module is ready to execute: parsing args may do that. */
  3254		after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
  3255					  -32768, 32767, mod,
  3256					  unknown_module_param_cb);
  3257		if (IS_ERR(after_dashes)) {
  3258			err = PTR_ERR(after_dashes);
  3259			goto coming_cleanup;
  3260		} else if (after_dashes) {
  3261			pr_warn("%s: parameters '%s' after `--' ignored\n",
  3262			       mod->name, after_dashes);
  3263		}
  3264	
  3265		/* Link in to sysfs. */
  3266		err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
  3267		if (err < 0)
  3268			goto coming_cleanup;
  3269	
  3270		if (is_livepatch_module(mod)) {
  3271			err = copy_module_elf(mod, info);
  3272			if (err < 0)
  3273				goto sysfs_cleanup;
  3274		}
  3275	
  3276		/* Get rid of temporary copy. */
  3277		free_copy(info, flags);
  3278	
  3279		codetag_load_module(mod);
  3280	
  3281		/* Done! */
  3282		trace_module_load(mod);
  3283	
  3284		return do_init_module(mod);
  3285	
  3286	 sysfs_cleanup:
  3287		mod_sysfs_teardown(mod);
  3288	 coming_cleanup:
  3289		mod->state = MODULE_STATE_GOING;
  3290		destroy_params(mod->kp, mod->num_kp);
  3291		blocking_notifier_call_chain(&module_notify_list,
  3292					     MODULE_STATE_GOING, mod);
  3293		klp_module_going(mod);
  3294	 bug_cleanup:
  3295		mod->state = MODULE_STATE_GOING;
  3296		/* module_bug_cleanup needs module_mutex protection */
  3297		mutex_lock(&module_mutex);
  3298		module_bug_cleanup(mod);
  3299		mutex_unlock(&module_mutex);
  3300	
  3301	 ddebug_cleanup:
  3302		ftrace_release_mod(mod);
  3303		synchronize_rcu();
  3304		kfree(mod->args);
  3305	 free_arch_cleanup:
  3306		module_arch_cleanup(mod);
  3307	 free_modinfo:
  3308		free_modinfo(mod);
  3309	 free_unload:
  3310		module_unload_free(mod);
  3311	 unlink_mod:
  3312		mutex_lock(&module_mutex);
  3313		/* Unlink carefully: kallsyms could be walking list. */
  3314		list_del_rcu(&mod->list);
  3315		mod_tree_remove(mod);
  3316		wake_up_all(&module_wq);
  3317		/* Wait for RCU-sched synchronizing before releasing mod->list. */
  3318		synchronize_rcu();
  3319		mutex_unlock(&module_mutex);
  3320	 free_module:
  3321		mod_stat_bump_invalid(info, flags);
  3322		/* Free lock-classes; relies on the preceding sync_rcu() */
  3323		for_class_mod_mem_type(type, core_data) {
  3324			lockdep_free_key_range(mod->mem[type].base,
  3325					       mod->mem[type].size);
  3326		}
  3327	
  3328		module_deallocate(mod, info);
  3329	 free_copy:
  3330		/*
  3331		 * The info->len is always set. We distinguish between
  3332		 * failures once the proper module was allocated and
  3333		 * before that.
  3334		 */
  3335		if (!module_allocated) {
> 3336			audit_log_kern_module(info->name ? info->name : "(unavailable)");
  3337			mod_stat_bump_becoming(info, flags);
  3338		}
  3339		free_copy(info, flags);
  3340		return err;
  3341	}
  3342
diff mbox series

Patch

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 49b9bca9de12..1f482532ef66 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3057,8 +3057,10 @@  static int load_module(struct load_info *info, const char __user *uargs,
 	 * failures once the proper module was allocated and
 	 * before that.
 	 */
-	if (!module_allocated)
+	if (!module_allocated) {
+		audit_log_kern_module(info->name ? info->name : "(unavailable)");
 		mod_stat_bump_becoming(info, flags);
+	}
 	free_copy(info, flags);
 	return err;
 }