lkml.org 
[lkml]   [2018]   [Feb]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] audit: return on memory error to avoid null pointer dereference
On 2018-02-21 19:02, Paul Moore wrote:
> On Wed, Feb 21, 2018 at 6:49 PM, Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Feb 21, 2018 at 4:30 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> 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 <rgb@redhat.com>
> >> ---
> >> 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...)

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

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

\
 
 \ /
  Last update: 2018-02-22 02:00    [W:0.040 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site