audit: Use strscpy instead of memcpy when copying comm

Commit Message

Jinjie Ruan July 31, 2024, 7:52 a.m. UTC

There may be random garbage beyond a string's null terminator, memcpy might
use the entire comm array. so avoid that possibility by using strscpy
instead of memcpy.

Link: https://github.com/KSPP/linux/issues/90
Signed-off-by: Jinjie Ruan <[e-mail]>
---
 kernel/auditsc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Paul Moore July 31, 2024, 3:51 p.m. UTC | #1

On Wed, Jul 31, 2024 at 3:46 AM Jinjie Ruan <[e-mail]> wrote:
>
> There may be random garbage beyond a string's null terminator, memcpy might
> use the entire comm array. so avoid that possibility by using strscpy
> instead of memcpy.
>
> Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Jinjie Ruan <[e-mail]>
> ---
>  kernel/auditsc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

If you look at audit_log_pid_context() you'll see that we don't record
the entire task::comm field, we only record up the NUL byte, so any
garbage present after the end of the string should not make it into
the audit record.  We use memcpy(), as opposed to any of the string
based copy functions, as the task::comm field is relatively short and
having to count the length of the string in addition to copying the
string is likely more expensive than simply copying the full buffer.

Jinjie Ruan Aug. 1, 2024, 1:25 a.m. UTC | #2

On 2024/7/31 23:51, Paul Moore wrote:
> On Wed, Jul 31, 2024 at 3:46 AM Jinjie Ruan <[e-mail]> wrote:
>>
>> There may be random garbage beyond a string's null terminator, memcpy might
>> use the entire comm array. so avoid that possibility by using strscpy
>> instead of memcpy.
>>
>> Link: https://github.com/KSPP/linux/issues/90
>> Signed-off-by: Jinjie Ruan <[e-mail]>
>> ---
>>  kernel/auditsc.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> If you look at audit_log_pid_context() you'll see that we don't record
> the entire task::comm field, we only record up the NUL byte, so any
> garbage present after the end of the string should not make it into
> the audit record.  We use memcpy(), as opposed to any of the string
> based copy functions, as the task::comm field is relatively short and
> having to count the length of the string in addition to copying the
> string is likely more expensive than simply copying the full buffer.

Thank you very much, You're right, sorry I didn't read the code
carefully enough.

>

Patch

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6f0d6fb6523f..e4ef5e57dde9 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2730,7 +2730,7 @@  void __audit_ptrace(struct task_struct *t)
 	context->target_uid = task_uid(t);
 	context->target_sessionid = audit_get_sessionid(t);
 	security_task_getsecid_obj(t, &context->target_sid);
-	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
+	strscpy(context->target_comm, t->comm);
 }
 
 /**
@@ -2757,7 +2757,7 @@  int audit_signal_info_syscall(struct task_struct *t)
 		ctx->target_uid = t_uid;
 		ctx->target_sessionid = audit_get_sessionid(t);
 		security_task_getsecid_obj(t, &ctx->target_sid);
-		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
+		strscpy(ctx->target_comm, t->comm);
 		return 0;
 	}
 
@@ -2778,7 +2778,7 @@  int audit_signal_info_syscall(struct task_struct *t)
 	axp->target_uid[axp->pid_count] = t_uid;
 	axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
 	security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]);
-	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
+	strscpy(axp->target_comm[axp->pid_count], t->comm);
 	axp->pid_count++;
 
 	return 0;