* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2024-10-23 21:13 [PATCH v1] audit,module: restore audit logging in load failure case Richard Guy Briggs
@ 2024-10-24 17:58 ` kernel test robot
2024-10-24 20:41 ` Paul Moore
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-10-24 17:58 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List
Cc: llvm, oe-kbuild-all, Paul Moore, Eric Paris, Steve Grubb,
Richard Guy Briggs
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2024-10-23 21:13 [PATCH v1] audit,module: restore audit logging in load failure case Richard Guy Briggs
2024-10-24 17:58 ` kernel test robot
@ 2024-10-24 20:41 ` Paul Moore
2025-03-06 21:41 ` Richard Guy Briggs
2024-10-25 7:38 ` kernel test robot
2024-10-28 5:55 ` kernel test robot
3 siblings, 1 reply; 9+ messages in thread
From: Paul Moore @ 2024-10-24 20:41 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List
Cc: Eric Paris, Steve Grubb, Richard Guy Briggs
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2024-10-24 20:41 ` Paul Moore
@ 2025-03-06 21:41 ` Richard Guy Briggs
2025-03-07 0:11 ` Richard Guy Briggs
2025-03-07 19:41 ` Steve Grubb
0 siblings, 2 replies; 9+ messages in thread
From: Richard Guy Briggs @ 2025-03-06 21:41 UTC (permalink / raw)
To: Paul Moore
Cc: Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List, Eric Paris, Steve Grubb
On 2024-10-24 16:41, Paul Moore wrote:
> 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.
Fair enough.
> 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.
Understood. I wonder if the absence of the record in the error case
will leave us guessing if we lost a record from the event? We will have
the error code from the SYSCALL record but not much more than that, and
some of those error codes could just as well be generated after that
point too. This would be a similar situation to the vanishing fields in
an existing record, but is likely easier to mitigate than a
non-last-field vanishing or shifting around in an existing record.
Steve? Atilla? Any comments?
> 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
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2025-03-06 21:41 ` Richard Guy Briggs
@ 2025-03-07 0:11 ` Richard Guy Briggs
2025-03-07 19:41 ` Steve Grubb
1 sibling, 0 replies; 9+ messages in thread
From: Richard Guy Briggs @ 2025-03-07 0:11 UTC (permalink / raw)
To: Paul Moore
Cc: Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List, Eric Paris, Steve Grubb
On 2025-03-06 16:41, Richard Guy Briggs wrote:
> On 2024-10-24 16:41, Paul Moore wrote:
> > 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.
>
> Fair enough.
>
> > 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.
>
> Understood. I wonder if the absence of the record in the error case
> will leave us guessing if we lost a record from the event? We will have
> the error code from the SYSCALL record but not much more than that, and
> some of those error codes could just as well be generated after that
> point too. This would be a similar situation to the vanishing fields in
> an existing record, but is likely easier to mitigate than a
> non-last-field vanishing or shifting around in an existing record.
In fact, the reason the original issue was filed was due to the
unexpected missing record, so it seems that it would be best to include
it regardless so that event can be found more consistently by
KERN_MODULE record type rather than searching for SYSCALL sub-field.
> Steve? Atilla? Any comments?
>
> > 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
>
> - RGB
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2025-03-06 21:41 ` Richard Guy Briggs
2025-03-07 0:11 ` Richard Guy Briggs
@ 2025-03-07 19:41 ` Steve Grubb
2025-03-13 15:18 ` Richard Guy Briggs
1 sibling, 1 reply; 9+ messages in thread
From: Steve Grubb @ 2025-03-07 19:41 UTC (permalink / raw)
To: Paul Moore, Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List, Eric Paris
On Thursday, March 6, 2025 4:41:40 PM Eastern Standard Time Richard Guy
Briggs wrote:
> On 2024-10-24 16:41, Paul Moore wrote:
> > 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.
>
> Fair enough.
>
> > 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.
>
> Understood. I wonder if the absence of the record in the error case
> will leave us guessing if we lost a record from the event? We will have
> the error code from the SYSCALL record but not much more than that, and
> some of those error codes could just as well be generated after that
> point too. This would be a similar situation to the vanishing fields in
> an existing record, but is likely easier to mitigate than a
> non-last-field vanishing or shifting around in an existing record.
>
> Steve? Atilla? Any comments?
We should only get the events if we have syscall rules that would result in
kernel module loading/unloading events. I suppose that by the time audit
loads it's rules, many modules have already loaded. So, I don't think we need
to worry about early cases. But when we do get a kernel module load/unload
event based on the rules, we need it's name - even in failures. We need to be
able to determine what was attempted since this could affect system
integrity.
-Steve
> > 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
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> Upstream IRC: SunRaycer
> Voice: +1.613.860 2354 SMS: +1.613.518.6570
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2025-03-07 19:41 ` Steve Grubb
@ 2025-03-13 15:18 ` Richard Guy Briggs
0 siblings, 0 replies; 9+ messages in thread
From: Richard Guy Briggs @ 2025-03-13 15:18 UTC (permalink / raw)
To: Steve Grubb
Cc: Paul Moore, Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List, Eric Paris
On 2025-03-07 14:41, Steve Grubb wrote:
> On Thursday, March 6, 2025 4:41:40 PM Eastern Standard Time Richard Guy
> Briggs wrote:
> > On 2024-10-24 16:41, Paul Moore wrote:
> > > 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.
> >
> > Fair enough.
> >
> > > 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.
> >
> > Understood. I wonder if the absence of the record in the error case
> > will leave us guessing if we lost a record from the event? We will have
> > the error code from the SYSCALL record but not much more than that, and
> > some of those error codes could just as well be generated after that
> > point too. This would be a similar situation to the vanishing fields in
> > an existing record, but is likely easier to mitigate than a
> > non-last-field vanishing or shifting around in an existing record.
> >
> > Steve? Atilla? Any comments?
>
> We should only get the events if we have syscall rules that would result in
> kernel module loading/unloading events. I suppose that by the time audit
> loads it's rules, many modules have already loaded. So, I don't think we need
> to worry about early cases. But when we do get a kernel module load/unload
> event based on the rules, we need it's name - even in failures. We need to be
> able to determine what was attempted since this could affect system
> integrity.
Ok, yes, agreed. But that information isn't available yet for a number
of error cases. It hasn't even been looked for yet after
module_sig_check() and can't be reliably found until halfway through
elf_validity_cache_copy().
So, we should provide the record and make a best effort to fill in that
information if we can. We could try more aggressively to extract it
from the info blob, but I don't know if that is worth the effort because
a number of the steps leading up to it are necessary to check the
validity and set up structures to extract it.
> -Steve
>
> > > 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
> >
> > - RGB
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2024-10-23 21:13 [PATCH v1] audit,module: restore audit logging in load failure case Richard Guy Briggs
2024-10-24 17:58 ` kernel test robot
2024-10-24 20:41 ` Paul Moore
@ 2024-10-25 7:38 ` kernel test robot
2024-10-28 5:55 ` kernel test robot
3 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-10-25 7:38 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List
Cc: oe-kbuild-all, Paul Moore, Eric Paris, Steve Grubb,
Richard Guy Briggs
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v1] audit,module: restore audit logging in load failure case
2024-10-23 21:13 [PATCH v1] audit,module: restore audit logging in load failure case Richard Guy Briggs
` (2 preceding siblings ...)
2024-10-25 7:38 ` kernel test robot
@ 2024-10-28 5:55 ` kernel test robot
3 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2024-10-28 5:55 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-modules,
Linux Kernel Audit Mailing List
Cc: oe-kbuild-all, Paul Moore, Eric Paris, Steve Grubb,
Richard Guy Briggs
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread