Re: [PATCH] audit: fix error handling in audit_data_to_entry()

On Tue, Mar 17, 2020 at 3:14 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2020-02-24 16:31, Paul Moore wrote: > > Commit 219ca39427bf ("audit: use union for audit_field values since > > they are mutually exclusive") combined a number of separate fields in > > the audit_field struct into a single union. Generally this worked > > just fine because they are generally mutually exclusive. > > Unfortunately in audit_data_to_entry() the overlap can be a problem > > when a specific error case is triggered that causes the error path > > code to attempt to cleanup an audit_field struct and the cleanup > > involves attempting to free a stored LSM string (the lsm_str field). > > Currently the code always has a non-NULL value in the > > audit_field.lsm_str field as the top of the for-loop transfers a > > value into audit_field.val (both .lsm_str and .val are part of the > > same union); if audit_data_to_entry() fails and the audit_field > > struct is specified to contain a LSM string, but the > > audit_field.lsm_str has not yet been properly set, the error handling > > code will attempt to free the bogus audit_field.lsm_str value that > > was set with audit_field.val at the top of the for-loop. > > > > This patch corrects this by ensuring that the audit_field.val is only > > set when needed (it is cleared when the audit_field struct is > > allocated with kcalloc()). It also corrects a few other issues to > > ensure that in case of error the proper error code is returned. > > > > Cc: stable(a)vger.kernel.org > > Fixes: 219ca39427bf ("audit: use union for audit_field values since they are mutually exclusive") > > Reported-by: syzbot+1f4d90ead370d72e450b(a)syzkaller.appspotmail.com > > Signed-off-by: Paul Moore <paul(a)paul-moore.com&gt; > > --- > > kernel/auditfilter.c | 71 +++++++++++++++++++++++++++----------------------- > > 1 file changed, 39 insertions(+), 32 deletions(-) > > > > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c > > index b0126e9c0743..026e34da4ace 100644 > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -456,6 +456,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, > > bufp = data->buf; > > for (i = 0; i < data->field_count; i++) { > > struct audit_field *f = &entry->rule.fields[i]; > > + u32 f_val; > > > > err = -EINVAL; > > > > @@ -464,12 +465,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, > > goto exit_free; > > > > f->type = data->fields[i]; > > - f->val = data->values[i]; > > + f_val = data->values[i]; > > > > /* Support legacy tests for a valid loginuid */ > > - if ((f->type == AUDIT_LOGINUID) && (f->val == AUDIT_UID_UNSET)) { > > + if ((f->type == AUDIT_LOGINUID) && (f_val == AUDIT_UID_UNSET)) { > > f->type = AUDIT_LOGINUID_SET; > > - f->val = 0; > > + f_val = 0; > > entry->rule.pflags |= AUDIT_LOGINUID_LEGACY; > > } > > > > @@ -485,7 +486,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, > > case AUDIT_SUID: > > case AUDIT_FSUID: > > case AUDIT_OBJ_UID: > > - f->uid = make_kuid(current_user_ns(), f->val); > > + f->uid = make_kuid(current_user_ns(), f_val); > > if (!uid_valid(f->uid)) > > goto exit_free; > > break; > > @@ -494,11 +495,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, > > case AUDIT_SGID: > > case AUDIT_FSGID: > > case AUDIT_OBJ_GID: > > - f->gid = make_kgid(current_user_ns(), f->val); > > + f->gid = make_kgid(current_user_ns(), f_val); > > if (!gid_valid(f->gid)) > > goto exit_free; > > break; > > case AUDIT_ARCH: > > + f->val = f_val; > > entry->rule.arch_f = f; > > break; > > case AUDIT_SUBJ_USER: > > @@ -511,11 +513,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, > > case AUDIT_OBJ_TYPE: > > case AUDIT_OBJ_LEV_LOW: > > case AUDIT_OBJ_LEV_HIGH: > > - str = audit_unpack_string(&bufp, &remain, f->val); > > - if (IS_ERR(str)) > > + str = audit_unpack_string(&bufp, &remain, f_val); > > + if (IS_ERR(str)) { > > + err = PTR_ERR(str); > > goto exit_free; > > - entry->rule.buflen += f->val; > > - > > + } > > + entry->rule.buflen += f_val; > > + f->lsm_str = str; > > err = security_audit_rule_init(f->type, f->op, str, > > (void **)&f->lsm_rule); > > /* Keep currently invalid fields around in case they > > @@ -524,68 +528,71 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, > > pr_warn("audit rule for LSM \'%s\' is invalid\n", > > str); > > err = 0; > > - } > > - if (err) { > > - kfree(str); > > + } else if (err) > > If there is an error from security_audit_rule_init() other than -EINVAL > (which could become valid after a policy reload), would the str passed > to it not need to be freed before we goto exit_free? After audit_unpack_string() succeeds we store "str" in the audit_field struct which should be cleaned up by audit_free_rule() when we jump to exit_free.

> > goto exit_free; > > - } else > > - f->lsm_str = str; > > break; > > case AUDIT_WATCH: > > - str = audit_unpack_string(&bufp, &remain, f->val); > > - if (IS_ERR(str)) > > + str = audit_unpack_string(&bufp, &remain, f_val); > > + if (IS_ERR(str)) { > > + err = PTR_ERR(str); > > goto exit_free; > > - entry->rule.buflen += f->val; > > - > > - err = audit_to_watch(&entry->rule, str, f->val, f->op); > > + } > > + err = audit_to_watch(&entry->rule, str, f_val, f->op); > > if (err) { > > kfree(str); > > goto exit_free; > > } > > + entry->rule.buflen += f_val; > > break; > > case AUDIT_DIR: > > - str = audit_unpack_string(&bufp, &remain, f->val); > > - if (IS_ERR(str)) > > + str = audit_unpack_string(&bufp, &remain, f_val); > > + if (IS_ERR(str)) { > > + err = PTR_ERR(str); > > goto exit_free; > > - entry->rule.buflen += f->val; > > - > > + } > > err = audit_make_tree(&entry->rule, str, f->op); > > kfree(str); > > if (err) > > goto exit_free; > > + entry->rule.buflen += f_val; > > break; > > case AUDIT_INODE: > > + f->val = f_val; > > err = audit_to_inode(&entry->rule, f); > > if (err) > > goto exit_free; > > break; > > case AUDIT_FILTERKEY: > > - if (entry->rule.filterkey || f->val > AUDIT_MAX_KEY_LEN) > > + if (entry->rule.filterkey || f_val > AUDIT_MAX_KEY_LEN) > > goto exit_free; > > - str = audit_unpack_string(&bufp, &remain, f->val); > > - if (IS_ERR(str)) > > + str = audit_unpack_string(&bufp, &remain, f_val); > > + if (IS_ERR(str)) { > > + err = PTR_ERR(str); > > goto exit_free; > > - entry->rule.buflen += f->val; > > + } > > + entry->rule.buflen += f_val; > > entry->rule.filterkey = str; > > break; > > case AUDIT_EXE: > > - if (entry->rule.exe || f->val > PATH_MAX) > > + if (entry->rule.exe || f_val > PATH_MAX) > > goto exit_free; > > - str = audit_unpack_string(&bufp, &remain, f->val); > > + str = audit_unpack_string(&bufp, &remain, f_val); > > if (IS_ERR(str)) { > > err = PTR_ERR(str); > > goto exit_free; > > } > > - entry->rule.buflen += f->val; > > - > > - audit_mark = audit_alloc_mark(&entry->rule, str, f->val); > > + audit_mark = audit_alloc_mark(&entry->rule, str, f_val); > > if (IS_ERR(audit_mark)) { > > kfree(str); > > err = PTR_ERR(audit_mark); > > goto exit_free; > > } > > + entry->rule.buflen += f_val; > > entry->rule.exe = audit_mark; > > break; > > + default: > > + f->val = f_val; > > + break; > > } > > } -- paul moore www.paul-moore.com