linux-kernel - Re: [PATCH] audit: fix potential null dereference 'context->module.name'

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: <201807250855344461527@zte.com.cn>
Date:   Wed, 25 Jul 2018 08:55:34 +0800 (CST)
From:   <wang.yi59@....com.cn>
To:     <paul@...l-moore.com>
Cc:     <eparis@...hat.com>, <linux-audit@...hat.com>,
        <linux-kernel@...r.kernel.org>, <jiang.biao2@....com.cn>,
        <zhong.weidong@....com.cn>
Subject: Re: [PATCH] audit: fix potential null dereference 'context->module.name'

> On Tue, Jul 24, 2018 at 6:38 PM Eric Paris <eparis@...hat.com> wrote:
> > On Tue, 2018-07-24 at 15:55 -0400, Paul Moore wrote:
> > > On Tue, Jul 24, 2018 at 7:39 AM Eric Paris <eparis@...hat.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.
> >
> > On the load_module() code path passing the error up the stack would
> > cause the module to not be loaded, instead of being loaded with loss of
> > name information. I'd rather have the module not loaded and a
> > name=(null) record than it BE loaded and get name=(null)
> >
> > You've sold me on why Yi Wang's patch is good, but not on why we
> > wouldn't propagate the error up the stack on load/delete. (even if we
> > may choose to delete the module on OOM)
>
> For better or worse most (all?) of the various audit_log_*() functions
> don't return an error code, or if they do they aren't reliably checked
> by callers.  I agree with you that in a perfect world it would be nice
> if an audit failure here would block the operation, but that isn't how
> audit is typically used.
>
> We could presumably fail the record generation on an allocation
> failure and signal a lost record via audit_log_lost()?  Perhaps that's
> the best option as it would leave the decision up to administrator
> (continue without the record, printk, or panic).
Thanks everyone for your review and advice! I will submit a v2 patch soon
using kstrdup() and recording the lost with audit_log_lost().

---
Best wishes
Yi Wang