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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Feb 2018 16:34:08 -0800
From:   Eric Paris <eparis@...hat.com>
To:     Paul Moore <paul@...l-moore.com>,
        Richard Guy Briggs <rgb@...hat.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Steve Grubb <sgrubb@...hat.com>
Subject: Re: [PATCH] audit: return on memory error to avoid null pointer
 dereference

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.


On Wed, 2018-02-21 at 19:02 -0500, 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.
> 
> 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.
> 
> > > 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
> 
>