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

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?

> 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; > } > }