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.

Mot-CRs-fixed: (CR)
CVE-Fixed: CVE-2020-0444
Bug: 150693166

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: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jignesh Patel <jignesh@motorola.com>
Change-Id: I5204ca49bded240ae942a21ae004d9f2f8c9202f
Reviewed-on: https://gerrit.mot.com/1768866
SLTApproved: Slta Waiver
SME-Granted: SME Approvals Granted
Tested-by: Jira Key
Reviewed-by: Xiangpo Zhao <zhaoxp3@motorola.com>
Submit-Approved: Jira Key

diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index 8dd4063647c2c7db83a612a3dc307044a83fae4e..e83482c6d1ab270c2f2b2e121b8938aee4c5a301 100644 (file)
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -435,6 +435,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;
 
@@ -443,12 +444,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;
                }
 
@@ -464,7 +465,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;
@@ -473,12 +474,13 @@ 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_SESSIONID:
                case AUDIT_ARCH:
+                       f->val = f_val;
                        entry->rule.arch_f = f;
                        break;
                case AUDIT_SUBJ_USER:
@@ -491,11 +493,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
@@ -504,68 +508,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;
                }
        }