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

On 2024-10-24 16:41, Paul Moore wrote: > On Oct 23, 2024 Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com&gt; > > --- > > 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