Re: [PATCH v2] cgroup: align the comm length with TASK_COMM_LEN

[Kassey Li <quic_yingangl@quicinc.com>]

On 9/26/2022 10:42 AM, Steven Rostedt wrote:
> On Mon, 26 Sep 2022 10:18:55 +0800
> Kassey Li <quic_yingangl@quicinc.com> wrote:
> 
>>>> @@ -139,12 +139,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>>>>    		__entry->dst_level = dst_cgrp->level;
>>>>    		__assign_str(dst_path, path);
>>>>    		__entry->pid = task->pid;
>>>> -		__assign_str(comm, task->comm);
>>>> +		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
>> 	I think the problem is here, __assign_str using strcpy
>> 	the task->comm here tail is not '\0'
>> 	that's why it out of bounds access.
>>
> 
> If this is the case, then there's a lot more than just tracing that will
> break. There are other places in the kernel has used strcpy() on task->comm,
> and many more that do "%s" on task->comm, which would also crash on this.

You are right.

by re-check my local logs(arm64), we can see the src has '\0' as end of 
string.
but looks strcpy did not catch this and crossed.
I can not figure out how this could happen. if there is debug suggest, 
please help.


src: task->comm   SharedPreferenc   pid  28395
_____________________address|________0________4________8________C_0123456789ABCDEF
   NSD:0000::FFFFFFBD1B6C59D0|>72616853 72506465 72656665 00636E65 
SharedPreferenc.


dst: trace event buffer:
_____________________address|________0________4________8________C_0123456789ABCDEF
   NSD:0000::FFFFFFBCF744FFE0| 00090020 000B0029 706F742F 7070612D 
...).../top-app
   NSD:0000::FFFFFFBCF744FFF0| 61685300 50646572 65666572 636E6572 
.SharedPreferenc
   NSD:0000::FFFFFFBCF7450000|>52800101 97FD3A05 140000B3 AA1303E0 
...R.:..........

layout of the struct:

   [ND:0x0::0xFFFFFFBCF744FFC8] (struct 
trace_event_raw_cgroup_migrate)0xFFFFFFBCF744FFc8 = (
     [ND:0x0::0xFFFFFFBCF744FFC8] ent = (
       [ND:0x0::0xFFFFFFBCF744FFC8] type = 0x98,
       [ND:0x0::0xFFFFFFBCF744FFCA] flags = 0x1,
       [ND:0x0::0xFFFFFFBCF744FFCB] preempt_count = 0x1,
       [ND:0x0::0xFFFFFFBCF744FFCC] pid = 0x0773),
     [ND:0x0::0xFFFFFFBCF744FFD0] dst_root = 0x1,
     [ND:0x0::0xFFFFFFBCF744FFD4] dst_id = 0x6,
     [ND:0x0::0xFFFFFFBCF744FFD8] dst_level = 0x1,
     [ND:0x0::0xFFFFFFBCF744FFDC] pid = 28395 = 0x6EEB,
     [ND:0x0::0xFFFFFFBCF744FFE0] __data_loc_dst_path = 0x00090020 = '... ',
     [ND:0x0::0xFFFFFFBCF744FFE4] __data_loc_comm = 0x000B0029 = '...)',
     [ND:0x0::0xFFFFFFBCF744FFE8] __data_=_"/top-app")

name: cgroup_attach_task
ID: 152
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:int dst_root;	offset:8;	size:4;	signed:1;
	field:int dst_id;	offset:12;	size:4;	signed:1;
	field:int dst_level;	offset:16;	size:4;	signed:1;
	field:int pid;	offset:20;	size:4;	signed:1;
	field:__data_loc char[] dst_path;	offset:24;	size:4;	signed:0;
	field:__data_loc char[] comm;	offset:28;	size:4;	signed:0;


_____________________address|________0________4________8________C_0123456789ABCDEF
   NSD:0000::FFFFFFBCF744FFC0| 00656C64 0066D18D>01010098 00000773 
dle...f.....s...
   NSD:0000::FFFFFFBCF744FFD0| 00000001 00000006 00000001 00006EEB 
.............n..
   NSD:0000::FFFFFFBCF744FFE0| 00090020 000B0029 706F742F 7070612D 
...).../top-app
   NSD:0000::FFFFFFBCF744FFF0| 61685300 50646572 65666572 636E6572 
.SharedPreferenc
   NSD:0000::FFFFFFBCF7450000| 52800101 97FD3A05 140000B3 AA1303E0 
...R.:..........

> 
>> 	do you want to this version or just modify the memcpy or strncpy to do
>> with a known length ?  please give suggest so I can modify .
> 
> I'm guessing a problem exists elsewhere that makes it look like this is the
> issue. I suggest finding where the '\0' is dropped (if that is indeed the
> case).
> 
> -- Steve