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

[Eric Paris]

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?
(also this seems like a good place to use kstrdup, instead of
kmalloc+strcpy)

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 <***@zte.com.cn>
> Reviewed-by: Jiang Biao <***@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]

On Tue, Jul 24, 2018 at 7:39 AM Eric Paris <***@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 <***@zte.com.cn>
> > Reviewed-by: Jiang Biao <***@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

[Eric Paris]

On Tue, 2018-07-24 at 15:55 -0400, Paul Moore wrote:
> On Tue, Jul 24, 2018 at 7:39 AM Eric Paris <***@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.

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)

-Eric

> > (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 <***@zte.com.cn>
> > > Reviewed-by: Jiang Biao <***@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]

On Tue, Jul 24, 2018 at 6:38 PM Eric Paris <***@redhat.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 <***@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.
>
> 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).

--
paul moore
www.paul-moore.com

[w***@zte.com.cn]

> On Tue, Jul 24, 2018 at 6:38 PM Eric Paris <***@redhat.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 <***@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.
> >
> > 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