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

[Richard Guy Briggs]

On 2018-02-21 19:02, Paul Moore wrote:
> On Wed, Feb 21, 2018 at 6:49 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Wed, Feb 21, 2018 at 4:30 AM, Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>  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@xxxxxxxxxx>
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