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

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

[Yi Wang]

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(-)

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

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

[Richard Guy Briggs]

On 2018-07-25 10:26, 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.
> 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>

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

> ---
> 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(-)
> 
> 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;
>  }
>  

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
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

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

[Paul Moore]

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