Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1532411834-33775-1-git-send-email-wang.yi59@zte.com.cn> <450ab68cc6b7cebbf1b6292e3140932ea1ac9e57.camel@redhat.com>
In-Reply-To: <450ab68cc6b7cebbf1b6292e3140932ea1ac9e57.camel@redhat.com>
From:   Paul Moore <paul@paul-moore.com>
Date:   Tue, 24 Jul 2018 15:55:55 -0400
Message-ID: <CAHC9VhQy6Rakx6WZ236R+PjwtaZ2Sq9SuoGMC8iejOmhRbCM_w@mail.gmail.com>
Subject: Re: [PATCH] audit: fix potential null dereference 'context->module.name'
To:     Eric Paris <eparis@redhat.com>, wang.yi59@zte.com.cn
Cc:     linux-audit@redhat.com, linux-kernel@vger.kernel.org,
        jiang.biao2@zte.com.cn, zhong.weidong@zte.com.cn
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 24, 2018 at 7:39 AM Eric Paris <eparis@redhat.com> wrote:
> Would it make more sense to actually check for failure on allocation
> rather than try to remember to deal with it later? How about we just
> have audit_log_kern_module return an error and fail if we are OOM?

Generally I agree, checking on allocation is the right thing to do.
However, in this particular case the error recovery for a failed
allocation would likely ignore the record entirely, and I think a
module load record with a "(null)" module name is still better than no
module load record at all ... and yes, I understand that if the module
name allocation fails there is a good chance other allocations will
fail and we might not emit the record, but I'll take my chances that
the load is transient and the system is able to recover in a timely
manner.

> (also this seems like a good place to use kstrdup, instead of
> kmalloc+strcpy)

Yes, I agree.  Yi Wang, could you submit a v2 of this patch using kstrdup()?

> On Tue, 2018-07-24 at 13:57 +0800, Yi Wang wrote:
> > The variable 'context->module.name' may be null pointer when
> > kmalloc return null, so it's better to check it before using
> > to avoid null dereference.
> >
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
> > ---
> >  kernel/auditsc.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index e80459f..4830b83 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1272,8 +1272,12 @@ static void show_special(struct audit_context
> > *context, int *call_panic)
> >               break;
> >       case AUDIT_KERN_MODULE:
> >               audit_log_format(ab, "name=");
> > -             audit_log_untrustedstring(ab, context->module.name);
> > -             kfree(context->module.name);
> > +             if (context->module.name) {
> > +                     audit_log_untrustedstring(ab, context-
> > >module.name);
> > +                     kfree(context->module.name);
> > +             } else
> > +                     audit_log_format(ab, "(null)");
> > +
> >               break;
> >       }
> >       audit_log_end(ab);
> > @@ -2409,7 +2413,8 @@ void __audit_log_kern_module(char *name)
> >       struct audit_context *context = current->audit_context;
> >
> >       context->module.name = kmalloc(strlen(name) + 1, GFP_KERNEL);
> > -     strcpy(context->module.name, name);
> > +     if (context->module.name)
> > +             strcpy(context->module.name, name);
> >       context->type = AUDIT_KERN_MODULE;
> >  }
> >


-- 
paul moore
www.paul-moore.com