Skip to content

Commit 2ad3e17

Browse files
pcmoorepcmoore
committed
audit: fix error handling in audit_data_to_entry()
Commit 219ca39 ("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: 219ca39 ("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>
1 parent cb5172d commit 2ad3e17

1 file changed

Lines changed: 39 additions & 32 deletions

File tree

‎kernel/auditfilter.c‎

Lines changed: 39 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
456456
bufp = data->buf;
457457
for (i = 0; i < data->field_count; i++) {
458458
struct audit_field *f = &entry->rule.fields[i];
459+
u32 f_val;
459460

460461
err = -EINVAL;
461462

@@ -464,12 +465,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
464465
goto exit_free;
465466

466467
f->type = data->fields[i];
467-
f->val = data->values[i];
468+
f_val = data->values[i];
468469

469470
/* Support legacy tests for a valid loginuid */
470-
if ((f->type == AUDIT_LOGINUID) && (f->val == AUDIT_UID_UNSET)) {
471+
if ((f->type == AUDIT_LOGINUID) && (f_val == AUDIT_UID_UNSET)) {
471472
f->type = AUDIT_LOGINUID_SET;
472-
f->val = 0;
473+
f_val = 0;
473474
entry->rule.pflags |= AUDIT_LOGINUID_LEGACY;
474475
}
475476

@@ -485,7 +486,7 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
485486
case AUDIT_SUID:
486487
case AUDIT_FSUID:
487488
case AUDIT_OBJ_UID:
488-
f->uid = make_kuid(current_user_ns(), f->val);
489+
f->uid = make_kuid(current_user_ns(), f_val);
489490
if (!uid_valid(f->uid))
490491
goto exit_free;
491492
break;
@@ -494,11 +495,12 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
494495
case AUDIT_SGID:
495496
case AUDIT_FSGID:
496497
case AUDIT_OBJ_GID:
497-
f->gid = make_kgid(current_user_ns(), f->val);
498+
f->gid = make_kgid(current_user_ns(), f_val);
498499
if (!gid_valid(f->gid))
499500
goto exit_free;
500501
break;
501502
case AUDIT_ARCH:
503+
f->val = f_val;
502504
entry->rule.arch_f = f;
503505
break;
504506
case AUDIT_SUBJ_USER:
@@ -511,11 +513,13 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
511513
case AUDIT_OBJ_TYPE:
512514
case AUDIT_OBJ_LEV_LOW:
513515
case AUDIT_OBJ_LEV_HIGH:
514-
str = audit_unpack_string(&bufp, &remain, f->val);
515-
if (IS_ERR(str))
516+
str = audit_unpack_string(&bufp, &remain, f_val);
517+
if (IS_ERR(str)) {
518+
err = PTR_ERR(str);
516519
goto exit_free;
517-
entry->rule.buflen += f->val;
518-
520+
}
521+
entry->rule.buflen += f_val;
522+
f->lsm_str = str;
519523
err = security_audit_rule_init(f->type, f->op, str,
520524
(void **)&f->lsm_rule);
521525
/* 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,
524528
pr_warn("audit rule for LSM \'%s\' is invalid\n",
525529
str);
526530
err = 0;
527-
}
528-
if (err) {
529-
kfree(str);
531+
} else if (err)
530532
goto exit_free;
531-
} else
532-
f->lsm_str = str;
533533
break;
534534
case AUDIT_WATCH:
535-
str = audit_unpack_string(&bufp, &remain, f->val);
536-
if (IS_ERR(str))
535+
str = audit_unpack_string(&bufp, &remain, f_val);
536+
if (IS_ERR(str)) {
537+
err = PTR_ERR(str);
537538
goto exit_free;
538-
entry->rule.buflen += f->val;
539-
540-
err = audit_to_watch(&entry->rule, str, f->val, f->op);
539+
}
540+
err = audit_to_watch(&entry->rule, str, f_val, f->op);
541541
if (err) {
542542
kfree(str);
543543
goto exit_free;
544544
}
545+
entry->rule.buflen += f_val;
545546
break;
546547
case AUDIT_DIR:
547-
str = audit_unpack_string(&bufp, &remain, f->val);
548-
if (IS_ERR(str))
548+
str = audit_unpack_string(&bufp, &remain, f_val);
549+
if (IS_ERR(str)) {
550+
err = PTR_ERR(str);
549551
goto exit_free;
550-
entry->rule.buflen += f->val;
551-
552+
}
552553
err = audit_make_tree(&entry->rule, str, f->op);
553554
kfree(str);
554555
if (err)
555556
goto exit_free;
557+
entry->rule.buflen += f_val;
556558
break;
557559
case AUDIT_INODE:
560+
f->val = f_val;
558561
err = audit_to_inode(&entry->rule, f);
559562
if (err)
560563
goto exit_free;
561564
break;
562565
case AUDIT_FILTERKEY:
563-
if (entry->rule.filterkey || f->val > AUDIT_MAX_KEY_LEN)
566+
if (entry->rule.filterkey || f_val > AUDIT_MAX_KEY_LEN)
564567
goto exit_free;
565-
str = audit_unpack_string(&bufp, &remain, f->val);
566-
if (IS_ERR(str))
568+
str = audit_unpack_string(&bufp, &remain, f_val);
569+
if (IS_ERR(str)) {
570+
err = PTR_ERR(str);
567571
goto exit_free;
568-
entry->rule.buflen += f->val;
572+
}
573+
entry->rule.buflen += f_val;
569574
entry->rule.filterkey = str;
570575
break;
571576
case AUDIT_EXE:
572-
if (entry->rule.exe || f->val > PATH_MAX)
577+
if (entry->rule.exe || f_val > PATH_MAX)
573578
goto exit_free;
574-
str = audit_unpack_string(&bufp, &remain, f->val);
579+
str = audit_unpack_string(&bufp, &remain, f_val);
575580
if (IS_ERR(str)) {
576581
err = PTR_ERR(str);
577582
goto exit_free;
578583
}
579-
entry->rule.buflen += f->val;
580-
581-
audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
584+
audit_mark = audit_alloc_mark(&entry->rule, str, f_val);
582585
if (IS_ERR(audit_mark)) {
583586
kfree(str);
584587
err = PTR_ERR(audit_mark);
585588
goto exit_free;
586589
}
590+
entry->rule.buflen += f_val;
587591
entry->rule.exe = audit_mark;
588592
break;
593+
default:
594+
f->val = f_val;
595+
break;
589596
}
590597
}
591598

0 commit comments

Comments
 (0)