Re: [PATCH 4/7] fs/binfmt_elf: use get_task_comm instead of open-coded string copy

[David Hildenbrand <david@redhat.com>]

On 11.11.21 12:34, Petr Mladek wrote:
> On Thu 2021-11-11 11:06:04, David Hildenbrand wrote:
>> On 11.11.21 11:03, David Hildenbrand wrote:
>>> On 08.11.21 09:38, Yafang Shao wrote:
>>>> It is better to use get_task_comm() instead of the open coded string
>>>> copy as we do in other places.
>>>>
>>>> struct elf_prpsinfo is used to dump the task information in userspace
>>>> coredump or kernel vmcore. Below is the verfication of vmcore,
>>>>
>>>> crash> ps
>>>>    PID    PPID  CPU       TASK        ST  %MEM     VSZ    RSS  COMM
>>>>       0      0   0  ffffffff9d21a940  RU   0.0       0      0  [swapper/0]
>>>>>     0      0   1  ffffa09e40f85e80  RU   0.0       0      0  [swapper/1]
>>>>>     0      0   2  ffffa09e40f81f80  RU   0.0       0      0  [swapper/2]
>>>>>     0      0   3  ffffa09e40f83f00  RU   0.0       0      0  [swapper/3]
>>>>>     0      0   4  ffffa09e40f80000  RU   0.0       0      0  [swapper/4]
>>>>>     0      0   5  ffffa09e40f89f80  RU   0.0       0      0  [swapper/5]
>>>>       0      0   6  ffffa09e40f8bf00  RU   0.0       0      0  [swapper/6]
>>>>>     0      0   7  ffffa09e40f88000  RU   0.0       0      0  [swapper/7]
>>>>>     0      0   8  ffffa09e40f8de80  RU   0.0       0      0  [swapper/8]
>>>>>     0      0   9  ffffa09e40f95e80  RU   0.0       0      0  [swapper/9]
>>>>>     0      0  10  ffffa09e40f91f80  RU   0.0       0      0  [swapper/10]
>>>>>     0      0  11  ffffa09e40f93f00  RU   0.0       0      0  [swapper/11]
>>>>>     0      0  12  ffffa09e40f90000  RU   0.0       0      0  [swapper/12]
>>>>>     0      0  13  ffffa09e40f9bf00  RU   0.0       0      0  [swapper/13]
>>>>>     0      0  14  ffffa09e40f98000  RU   0.0       0      0  [swapper/14]
>>>>>     0      0  15  ffffa09e40f9de80  RU   0.0       0      0  [swapper/15]
>>>>
>>>> It works well as expected.
>>>>
>>>> Suggested-by: Kees Cook <keescook@chromium.org>
>>>> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>>>> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>>>> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
>>>> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
>>>> Cc: Michal Miroslaw <mirq-linux@rere.qmqm.pl>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: Steven Rostedt <rostedt@goodmis.org>
>>>> Cc: Matthew Wilcox <willy@infradead.org>
>>>> Cc: David Hildenbrand <david@redhat.com>
>>>> Cc: Al Viro <viro@zeniv.linux.org.uk>
>>>> Cc: Kees Cook <keescook@chromium.org>
>>>> Cc: Petr Mladek <pmladek@suse.com>
>>>> ---
>>>>  fs/binfmt_elf.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
>>>> index a813b70f594e..138956fd4a88 100644
>>>> --- a/fs/binfmt_elf.c
>>>> +++ b/fs/binfmt_elf.c
>>>> @@ -1572,7 +1572,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p,
>>>>  	SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid));
>>>>  	SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid));
>>>>  	rcu_read_unlock();
>>>> -	strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
>>>> +	get_task_comm(psinfo->pr_fname, p);
>>>>  
>>>>  	return 0;
>>>>  }
>>>>
>>>
>>> We have a hard-coded "pr_fname[16]" as well, not sure if we want to
>>> adjust that to use TASK_COMM_LEN?
>>
>> But if the intention is to chance TASK_COMM_LEN later, we might want to
>> keep that unchanged.
> 
> It seems that len will not change in the end. Another solution is
> going to be used for the long names, see
> https://lore.kernel.org/r/20211108084142.4692-1-laoar.shao@gmail.com.

Yes, that's what I recall as well. The I read the patch
subjects+descriptions in this series "make it adopt to task comm size
change" and was slightly confused.

Maybe we should just remove any notion of "task comm size change" from
this series and instead just call it a cleanup.


-- 
Thanks,

David / dhildenb