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: <1532485579-34431-1-git-send-email-wang.yi59@zte.com.cn>
In-Reply-To: <1532485579-34431-1-git-send-email-wang.yi59@zte.com.cn>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 30 Jul 2018 19:04:18 -0400
Message-ID: <CAHC9VhRo8xohn+iayiEet54tUPGjQxzQ4oqCD3WSGX=9X_YHsA@mail.gmail.com>
Subject: Re: [PATCH v2] audit: fix potential null dereference 'context->module.name'
To:     wang.yi59@zte.com.cn
Cc:     Eric Paris <eparis@redhat.com>, 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 10:28 PM Yi Wang <wang.yi59@zte.com.cn> 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.
> Another one more thing this patch does is using kstrdup instead
> of (kmalloc + strcpy), and signal a lost record via audit_log_lost.
>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> Reviewed-by: Jiang Biao <jiang.biao2@zte.com.cn>
> ---
> v2: use kstrdup instead of kmalloc + strcpy, and signal a lost
> record. Thanks to Eric and Paul.
>
>  kernel/auditsc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Thanks, this looks good to me.  I'm also going to tag this for -stable.

I'm building a test kernel right now and if all goes well I'll send
this up to Linus for v4.18; if he doesn't pull it for v4.18 I'll add
this to the audit/next branch.

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index e80459f..713386a 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);
> @@ -2408,8 +2412,9 @@ 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);
> +       context->module.name = kstrdup(name, GFP_KERNEL);
> +       if (!context->module.name)
> +               audit_log_lost("out of memory in __audit_log_kern_module");
>         context->type = AUDIT_KERN_MODULE;
>  }
>
> --
> 1.8.3.1
>


-- 
paul moore
www.paul-moore.com