[PATCH] audit: fix error handling in audit_data_to_entry()
Paul Moore
paul at paul-moore.com
Tue Mar 17 20:37:04 UTC 2020
On Tue, Mar 17, 2020 at 3:14 PM Richard Guy Briggs <rgb at redhat.com> 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 at vger.kernel.org
> > Fixes: 219ca39427bf ("audit: use union for audit_field values since they are mutually exclusive")
> > Reported-by: syzbot+1f4d90ead370d72e450b at syzkaller.appspotmail.com
> > Signed-off-by: Paul Moore <paul at paul-moore.com>
> > ---
> > 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
More information about the Linux-audit
mailing list