[PATCH] audit: return on memory error to avoid null pointer dereference

[Richard Guy Briggs]

If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer. Return
instead.

See: https://github.com/linux-audit/audit-kernel/issues/76
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 196d327..31cb11d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1063,6 +1063,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
return;

ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,

[Richard Guy Briggs]

Post by Richard Guy Briggs
If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer. Return
instead.
See: https://github.com/linux-audit/audit-kernel/issues/76

Self-NACK. It was based on local development and won't compile on
upstream. Fix pending.

Post by Richard Guy Briggs
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/audit.c b/kernel/audit.c
index 196d327..31cb11d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1063,6 +1063,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
return;
ab = audit_log_start(context, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
--
1.8.3.1

- RGB

--
Richard Guy Briggs <***@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

[Richard Guy Briggs]

If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer. Return
instead.

Passes audit-testsuite.
See: https://github.com/linux-audit/audit-kernel/issues/76
Signed-off-by: Richard Guy Briggs <***@redhat.com>
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/kernel/audit.c b/kernel/audit.c
index 5c25449..2de74be 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
return;

ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,

[Paul Moore]

Post by Richard Guy Briggs
If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer. Return
instead.
Passes audit-testsuite.
See: https://github.com/linux-audit/audit-kernel/issues/76
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)

Thanks, merged.

In the future a "[PATCH v2]" prefix would be appreciated for patches
like this, it makes things a little easier in my inbox.

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 5c25449..2de74be 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
return;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
--
1.8.3.1

[Paul Moore]

Post by Paul Moore

Post by Richard Guy Briggs
If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer. Return
instead.
Passes audit-testsuite.
See: https://github.com/linux-audit/audit-kernel/issues/76
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)

Thanks, merged.
In the future a "[PATCH v2]" prefix would be appreciated for patches
like this, it makes things a little easier in my inbox.

After merging this I went through all the other callers to see if they
suffered the same mistake and everyone except for IMA was checking the
returned pointer for NULL. Upon looking at the IMA code, and the
audit code which is called, I realized we are actually "ok" as
audit_log_task_info(), audit_log_format(), audit_log_end(), and others
all check for a NULL audit_buffer at the very top of the functions.
I'm going to leave this patch merged, it's a good practice after all,
but I don't believe that unpatched systems are in any danger of
oops'ing here.

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 5c25449..2de74be 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
return;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
--
1.8.3.1

[Eric Paris]

I think if we went back and looked at history we'd see that all of the
code originally had none of the if(!ab) checks after allocation and
they just sorta slowly crept in over time. I prefer this pattern, but
it used to be the opposite everywhere.

Post by Paul Moore

Post by Paul Moore

Post by Richard Guy Briggs
If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that
pointer. Return
instead.
Passes audit-testsuite.
See: https://github.com/linux-audit/audit-kernel/issues/76
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)

Thanks, merged.
In the future a "[PATCH v2]" prefix would be appreciated for
patches
like this, it makes things a little easier in my inbox.

After merging this I went through all the other callers to see if they
suffered the same mistake and everyone except for IMA was checking the
returned pointer for NULL. Upon looking at the IMA code, and the
audit code which is called, I realized we are actually "ok" as
audit_log_task_info(), audit_log_format(), audit_log_end(), and others
all check for a NULL audit_buffer at the very top of the functions.
I'm going to leave this patch merged, it's a good practice after all,
but I don't believe that unpatched systems are in any danger of
oops'ing here.

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 5c25449..2de74be 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int
which, u32 old_feature, u32 new_feature
return;
ab = audit_log_start(NULL, GFP_KERNEL,
AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u
old_lock=%u new_lock=%u res=%d",
audit_feature_names[which],
!!old_feature, !!new_feature,
--
1.8.3.1

[Richard Guy Briggs]

Post by Paul Moore

Post by Paul Moore

Post by Richard Guy Briggs
If there is a memory allocation error when trying to change an audit
kernel feature value, the ignored allocation error will trigger a NULL
pointer dereference oops on subsequent use of that pointer. Return
instead.
Passes audit-testsuite.
See: https://github.com/linux-audit/audit-kernel/issues/76
---
kernel/audit.c | 2 ++
1 file changed, 2 insertions(+)

Thanks, merged.
In the future a "[PATCH v2]" prefix would be appreciated for patches
like this, it makes things a little easier in my inbox.

(Sorry, forgot in haste to get that fixed one out...)

Post by Paul Moore
After merging this I went through all the other callers to see if they
suffered the same mistake and everyone except for IMA was checking the
returned pointer for NULL. Upon looking at the IMA code, and the
audit code which is called, I realized we are actually "ok" as
audit_log_task_info(), audit_log_format(), audit_log_end(), and others
all check for a NULL audit_buffer at the very top of the functions.
I'm going to leave this patch merged, it's a good practice after all,
but I don't believe that unpatched systems are in any danger of
oops'ing here.

On review, agreed. My ctags/cscope DBs need regeneration, so I hadn't
noticed that the functions to which I was led weren't the ones I was
seeking, and while these three do check, not all functions that
accept a struct audit_buffer pointer parameter don't check for NULL.
Now that I check, I only find audit_expand (whose callers are all
protected) and audit_log_d_path (whose callers all appear to be
protected), the latter of which I've spent a bit of time staring at of
late (ghak8, ghak21...).

We're ok.

Post by Paul Moore

Post by Paul Moore

Post by Richard Guy Briggs
diff --git a/kernel/audit.c b/kernel/audit.c
index 5c25449..2de74be 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1059,6 +1059,8 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
return;
ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ if (!ab)
+ return;
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
--
1.8.3.1

--
paul moore
www.paul-moore.com

- RGB

--
Richard Guy Briggs <***@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