| From 9ab6f16368bef1879bd79bec13b6fef3fab6d110 Mon Sep 17 00:00:00 2001 |
| From: Paul Moore <paul@paul-moore.com> |
| Date: Sat, 22 Feb 2020 20:36:47 -0500 |
| Subject: [PATCH] audit: fix error handling in audit_data_to_entry() |
| |
| commit 2ad3e17ebf94b7b7f3f64c050ff168f9915345eb upstream. |
| |
| 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@vger.kernel.org |
| Fixes: 219ca39427bf ("audit: use union for audit_field values since they are mutually exclusive") |
| Reported-by: syzbot+1f4d90ead370d72e450b@syzkaller.appspotmail.com |
| Signed-off-by: Paul Moore <paul@paul-moore.com> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c |
| index 9f8e190e3bea..50f83e319da0 100644 |
| --- a/kernel/auditfilter.c |
| +++ b/kernel/auditfilter.c |
| @@ -439,6 +439,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; |
| |
| @@ -447,12 +448,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; |
| } |
| |
| @@ -468,7 +469,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; |
| @@ -477,11 +478,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: |
| @@ -494,11 +496,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 |
| @@ -507,68 +511,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) |
| 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; |
| } |
| } |
| |
| -- |
| 2.7.4 |
| |