linux-kernel - Re: [PATCH] audit: return on memory error to avoid null pointer dereference

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20180222005549.m45nkj5n6wbshbdl@madcap2.tricolour.ca>
Date:   Wed, 21 Feb 2018 19:55:49 -0500
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Eric Paris <eparis@...hat.com>, Steve Grubb <sgrubb@...hat.com>
Subject: Re: [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@...l-moore.com> wrote:
> > On Wed, Feb 21, 2018 at 4:30 AM, Richard Guy Briggs <rgb@...hat.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@...hat.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@...hat.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