netdev - Re: [PATCH resend v4 00/11] Improve the copy of task comm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87bk2gzgu0.fsf@intel.com>
Date: Mon, 29 Jul 2024 12:29:43 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Yafang Shao <laoar.shao@...il.com>, akpm@...ux-foundation.org
Cc: torvalds@...ux-foundation.org, ebiederm@...ssion.com,
 alexei.starovoitov@...il.com, rostedt@...dmis.org,
 catalin.marinas@....com, penguin-kernel@...ove.sakura.ne.jp,
 linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org, audit@...r.kernel.org,
 linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
 bpf@...r.kernel.org, netdev@...r.kernel.org,
 dri-devel@...ts.freedesktop.org, Yafang Shao <laoar.shao@...il.com>
Subject: Re: [PATCH resend v4 00/11] Improve the copy of task comm

On Mon, 29 Jul 2024, Yafang Shao <laoar.shao@...il.com> wrote:
> Hello Andrew,
>
> Is it appropriate for you to apply this to the mm tree?
>
> Using {memcpy,strncpy,strcpy,kstrdup} to copy the task comm relies on the
> length of task comm. Changes in the task comm could result in a destination
> string that is overflow. Therefore, we should explicitly ensure the destination
> string is always NUL-terminated, regardless of the task comm. This approach
> will facilitate future extensions to the task comm.

Why are we normalizing calling double-underscore prefixed functions all
over the place? i.e. __get_task_comm().

get_task_comm() is widely used. At a glance, looks like it could be used
in many of the patches here too.


BR,
Jani.


>
> As suggested by Linus [0], we can identify all relevant code with the
> following git grep command:
>
>   git grep 'memcpy.*->comm\>'
>   git grep 'kstrdup.*->comm\>'
>   git grep 'strncpy.*->comm\>'
>   git grep 'strcpy.*->comm\>'
>
> PATCH #2~#4:   memcpy
> PATCH #5~#6:   kstrdup
> PATCH #7~#9:   strncpy
> PATCH #10~#11: strcpy
>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0]
>
> Changes:
> v3->v4:
> - Rename __kstrndup() to __kmemdup_nul() and define it inside mm/util.c
>   (Matthew)
> - Remove unused local varaible (Simon)
>
> v2->v3: https://lore.kernel.org/all/20240621022959.9124-1-laoar.shao@gmail.com/
> - Deduplicate code around kstrdup (Andrew)
> - Add commit log for dropping task_lock (Catalin)
>
> v1->v2: https://lore.kernel.org/bpf/20240613023044.45873-1-laoar.shao@gmail.com/
> - Add comment for dropping task_lock() in __get_task_comm() (Alexei)
> - Drop changes in trace event (Steven)
> - Fix comment on task comm (Matus)
>
> v1: https://lore.kernel.org/all/20240602023754.25443-1-laoar.shao@gmail.com/
>
> Yafang Shao (11):
>   fs/exec: Drop task_lock() inside __get_task_comm()
>   auditsc: Replace memcpy() with __get_task_comm()
>   security: Replace memcpy() with __get_task_comm()
>   bpftool: Ensure task comm is always NUL-terminated
>   mm/util: Fix possible race condition in kstrdup()
>   mm/util: Deduplicate code in {kstrdup,kstrndup,kmemdup_nul}
>   mm/kmemleak: Replace strncpy() with __get_task_comm()
>   tsacct: Replace strncpy() with __get_task_comm()
>   tracing: Replace strncpy() with __get_task_comm()
>   net: Replace strcpy() with __get_task_comm()
>   drm: Replace strcpy() with __get_task_comm()
>
>  drivers/gpu/drm/drm_framebuffer.c     |  2 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c |  2 +-
>  fs/exec.c                             | 10 ++++-
>  include/linux/sched.h                 |  4 +-
>  kernel/auditsc.c                      |  6 +--
>  kernel/trace/trace.c                  |  2 +-
>  kernel/trace/trace_events_hist.c      |  2 +-
>  kernel/tsacct.c                       |  2 +-
>  mm/kmemleak.c                         |  8 +---
>  mm/util.c                             | 61 ++++++++++++---------------
>  net/ipv6/ndisc.c                      |  2 +-
>  security/lsm_audit.c                  |  4 +-
>  security/selinux/selinuxfs.c          |  2 +-
>  tools/bpf/bpftool/pids.c              |  2 +
>  14 files changed, 51 insertions(+), 58 deletions(-)

-- 
Jani Nikula, Intel