linux-audit.redhat.com archive mirror
 help / color / mirror / Atom feed
* [PATCH] audit: fix error handling in audit_data_to_entry()
@ 2020-02-24 21:31 Paul Moore
  2020-02-24 21:31 ` Paul Moore
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Paul Moore @ 2020-02-24 21:31 UTC (permalink / raw)
  To: linux-audit

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH] audit: fix error handling in audit_data_to_entry()
  2020-02-24 21:31 [PATCH] audit: fix error handling in audit_data_to_entry() Paul Moore
@ 2020-02-24 21:31 ` Paul Moore
  2020-02-25 17:58 ` Paul Moore
  2020-03-17 19:13 ` Richard Guy Briggs
  2 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2020-02-24 21:31 UTC (permalink / raw)
  To: linux-audit

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: fix error handling in audit_data_to_entry()
  2020-02-24 21:31 [PATCH] audit: fix error handling in audit_data_to_entry() Paul Moore
  2020-02-24 21:31 ` Paul Moore
@ 2020-02-25 17:58 ` Paul Moore
  2020-02-25 17:58   ` Paul Moore
  2020-03-17 19:13 ` Richard Guy Briggs
  2 siblings, 1 reply; 7+ messages in thread
From: Paul Moore @ 2020-02-25 17:58 UTC (permalink / raw)
  To: linux-audit

On Mon, Feb 24, 2020 at 4:31 PM Paul Moore <paul@paul-moore.com> 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@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>
> ---
>  kernel/auditfilter.c |   71 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)

Merged into audit/stable-5.6, assuming there are no problems I'll send
it up to Linus later this week.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: fix error handling in audit_data_to_entry()
  2020-02-25 17:58 ` Paul Moore
@ 2020-02-25 17:58   ` Paul Moore
  0 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2020-02-25 17:58 UTC (permalink / raw)
  To: linux-audit

On Mon, Feb 24, 2020 at 4:31 PM Paul Moore <paul@paul-moore.com> 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@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>
> ---
>  kernel/auditfilter.c |   71 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 32 deletions(-)

Merged into audit/stable-5.6, assuming there are no problems I'll send
it up to Linus later this week.

-- 
paul moore
www.paul-moore.com


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: fix error handling in audit_data_to_entry()
  2020-02-24 21:31 [PATCH] audit: fix error handling in audit_data_to_entry() Paul Moore
  2020-02-24 21:31 ` Paul Moore
  2020-02-25 17:58 ` Paul Moore
@ 2020-03-17 19:13 ` Richard Guy Briggs
  2020-03-17 20:37   ` Paul Moore
  2 siblings, 1 reply; 7+ messages in thread
From: Richard Guy Briggs @ 2020-03-17 19:13 UTC (permalink / raw)
  To: Paul Moore; +Cc: linux-audit

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@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>
> ---
>  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?

I think we need a
				kfree(str);
here (with braces, of course).

>  				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;
>  		}
>  	}
>  
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: fix error handling in audit_data_to_entry()
  2020-03-17 19:13 ` Richard Guy Briggs
@ 2020-03-17 20:37   ` Paul Moore
  2020-03-17 20:42     ` Richard Guy Briggs
  0 siblings, 1 reply; 7+ messages in thread
From: Paul Moore @ 2020-03-17 20:37 UTC (permalink / raw)
  To: Richard Guy Briggs; +Cc: linux-audit

On Tue, Mar 17, 2020 at 3:14 PM Richard Guy Briggs <rgb@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@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>
> > ---
> >  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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] audit: fix error handling in audit_data_to_entry()
  2020-03-17 20:37   ` Paul Moore
@ 2020-03-17 20:42     ` Richard Guy Briggs
  0 siblings, 0 replies; 7+ messages in thread
From: Richard Guy Briggs @ 2020-03-17 20:42 UTC (permalink / raw)
  To: Paul Moore; +Cc: linux-audit

On 2020-03-17 16:37, Paul Moore wrote:
> On Tue, Mar 17, 2020 at 3:14 PM Richard Guy Briggs <rgb@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@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>
> > > ---
> > >  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.

Got it, cool.  Everything else looked in place.  Looks good to me.

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

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-03-17 20:42 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-24 21:31 [PATCH] audit: fix error handling in audit_data_to_entry() Paul Moore
2020-02-24 21:31 ` Paul Moore
2020-02-25 17:58 ` Paul Moore
2020-02-25 17:58   ` Paul Moore
2020-03-17 19:13 ` Richard Guy Briggs
2020-03-17 20:37   ` Paul Moore
2020-03-17 20:42     ` Richard Guy Briggs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).