Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit c3395347 authored by Paul Moore's avatar Paul Moore Committed by Gerrit - the friendly Code Review server
Browse files

audit: fix error handling in audit_data_to_entry() 



Commit 219ca394 ("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: 219ca394 ("audit: use union for audit_field values since they are mutually exclusive")
Reported-by: default avatar <syzbot+1f4d90ead370d72e450b@syzkaller.appspotmail.com>
Signed-off-by: default avatarPaul Moore <paul@paul-moore.com>
Change-Id: Ic46b21eb3b20bbe1dc9f405ceb870e8c317b4d78
Git-repo: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git


Git-commit: 2ad3e17ebf94b7b7f3f64c050ff168f9915345eb
Signed-off-by: default avatarAlam Md Danish <amddan@codeaurora.org>
Signed-off-by: default avatarRahul Shahare <rshaha@codeaurora.org>
parent 39b81f8a

kernel/auditfilter.c

+39 −32
Original line number Original line Diff line number Diff line
@@ -434,6 +434,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, 
	bufp = data->buf;

	bufp = data->buf;

	for (i = 0; i < data->field_count; i++) {

	for (i = 0; i < data->field_count; i++) {

		struct audit_field *f = &entry->rule.fields[i];

		struct audit_field *f = &entry->rule.fields[i];

		u32 f_val;





		err = -EINVAL;

		err = -EINVAL;
@@ -442,12 +443,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, 
			goto exit_free;

			goto exit_free;





		f->type = data->fields[i];

		f->type = data->fields[i];

		f->val = data->values[i];

		f_val = data->values[i];





		/* Support legacy tests for a valid loginuid */

		/* 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->type = AUDIT_LOGINUID_SET;

			f->val = 0;

			f_val = 0;

			entry->rule.pflags |= AUDIT_LOGINUID_LEGACY;

			entry->rule.pflags |= AUDIT_LOGINUID_LEGACY;

		}

		}
@@ -463,7 +464,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, 
		case AUDIT_SUID:

		case AUDIT_SUID:

		case AUDIT_FSUID:

		case AUDIT_FSUID:

		case AUDIT_OBJ_UID:

		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))

			if (!uid_valid(f->uid))

				goto exit_free;

				goto exit_free;

			break;

			break;
@@ -472,11 +473,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, 
		case AUDIT_SGID:

		case AUDIT_SGID:

		case AUDIT_FSGID:

		case AUDIT_FSGID:

		case AUDIT_OBJ_GID:

		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))

			if (!gid_valid(f->gid))

				goto exit_free;

				goto exit_free;

			break;

			break;

		case AUDIT_ARCH:

		case AUDIT_ARCH:

			f->val = f_val;

			entry->rule.arch_f = f;

			entry->rule.arch_f = f;

			break;

			break;

		case AUDIT_SUBJ_USER:

		case AUDIT_SUBJ_USER:
@@ -489,11 +491,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, 
		case AUDIT_OBJ_TYPE:

		case AUDIT_OBJ_TYPE:

		case AUDIT_OBJ_LEV_LOW:

		case AUDIT_OBJ_LEV_LOW:

		case AUDIT_OBJ_LEV_HIGH:

		case AUDIT_OBJ_LEV_HIGH:

			str = audit_unpack_string(&bufp, &remain, f->val);

			str = audit_unpack_string(&bufp, &remain, f_val);

			if (IS_ERR(str))

			if (IS_ERR(str)) {

				err = PTR_ERR(str);

				goto exit_free;

				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,

			err = security_audit_rule_init(f->type, f->op, str,

						       (void **)&f->lsm_rule);

						       (void **)&f->lsm_rule);

			/* Keep currently invalid fields around in case they

			/* Keep currently invalid fields around in case they
@@ -502,68 +506,71 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data, 
				pr_warn("audit rule for LSM \'%s\' is invalid\n",

				pr_warn("audit rule for LSM \'%s\' is invalid\n",

					str);

					str);

				err = 0;

				err = 0;

			}

			} else if (err)

			if (err) {

				kfree(str);

				goto exit_free;

				goto exit_free;

			} else

				f->lsm_str = str;

			break;

			break;

		case AUDIT_WATCH:

		case AUDIT_WATCH:

			str = audit_unpack_string(&bufp, &remain, f->val);

			str = audit_unpack_string(&bufp, &remain, f_val);

			if (IS_ERR(str))

			if (IS_ERR(str)) {

				err = PTR_ERR(str);

				goto exit_free;

				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) {

			if (err) {

				kfree(str);

				kfree(str);

				goto exit_free;

				goto exit_free;

			}

			}

			entry->rule.buflen += f_val;

			break;

			break;

		case AUDIT_DIR:

		case AUDIT_DIR:

			str = audit_unpack_string(&bufp, &remain, f->val);

			str = audit_unpack_string(&bufp, &remain, f_val);

			if (IS_ERR(str))

			if (IS_ERR(str)) {

				err = PTR_ERR(str);

				goto exit_free;

				goto exit_free;

			entry->rule.buflen += f->val;

			}



			err = audit_make_tree(&entry->rule, str, f->op);

			err = audit_make_tree(&entry->rule, str, f->op);

			kfree(str);

			kfree(str);

			if (err)

			if (err)

				goto exit_free;

				goto exit_free;

			entry->rule.buflen += f_val;

			break;

			break;

		case AUDIT_INODE:

		case AUDIT_INODE:

			f->val = f_val;

			err = audit_to_inode(&entry->rule, f);

			err = audit_to_inode(&entry->rule, f);

			if (err)

			if (err)

				goto exit_free;

				goto exit_free;

			break;

			break;

		case AUDIT_FILTERKEY:

		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;

				goto exit_free;

			str = audit_unpack_string(&bufp, &remain, f->val);

			str = audit_unpack_string(&bufp, &remain, f_val);

			if (IS_ERR(str))

			if (IS_ERR(str)) {

				err = PTR_ERR(str);

				goto exit_free;

				goto exit_free;

			entry->rule.buflen += f->val;

			}

			entry->rule.buflen += f_val;

			entry->rule.filterkey = str;

			entry->rule.filterkey = str;

			break;

			break;

		case AUDIT_EXE:

		case AUDIT_EXE:

			if (entry->rule.exe || f->val > PATH_MAX)

			if (entry->rule.exe || f_val > PATH_MAX)

				goto exit_free;

				goto exit_free;

			str = audit_unpack_string(&bufp, &remain, f->val);

			str = audit_unpack_string(&bufp, &remain, f_val);

			if (IS_ERR(str)) {

			if (IS_ERR(str)) {

				err = PTR_ERR(str);

				err = PTR_ERR(str);

				goto exit_free;

				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)) {

			if (IS_ERR(audit_mark)) {

				kfree(str);

				kfree(str);

				err = PTR_ERR(audit_mark);

				err = PTR_ERR(audit_mark);

				goto exit_free;

				goto exit_free;

			}

			}

			entry->rule.buflen += f_val;

			entry->rule.exe = audit_mark;

			entry->rule.exe = audit_mark;

			break;

			break;

		default:

			f->val = f_val;

			break;

		}

		}

	}

	}