| Message ID | 20240729023719.1933-1-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve the copy of task comm | expand |
On Mon, 29 Jul 2024, Yafang Shao <laoar.shao@gmail.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@linux-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(-)
On Mon, Jul 29, 2024 at 5:29 PM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Mon, 29 Jul 2024, Yafang Shao <laoar.shao@gmail.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. There is a BUILD_BUG_ON() inside get_task_comm(), so when you use get_task_comm(), it implies that the BUILD_BUG_ON() is necessary. However, we don't want to impose this restriction on code where the length can be dynamically changed. One use case of get_task_comm() is in code that has already exposed the length to userspace. In such cases, we specifically add the BUILD_BUG_ON() to prevent developers from changing it. For more information, see commit 95af469c4f60 ("fs/binfmt_elf: replace open-coded string copy with get_task_comm").
On Mon, 29 Jul 2024 10:37:08 +0800 Yafang Shao <laoar.shao@gmail.com> wrote:
> Is it appropriate for you to apply this to the mm tree?
There are a couple of minor conflicts against current 6.11-rc1 which
you'd best check. So please redo this against current mainline?
On Wed, Jul 31, 2024 at 8:59 AM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Mon, 29 Jul 2024 10:37:08 +0800 Yafang Shao <laoar.shao@gmail.com> wrote: > > > Is it appropriate for you to apply this to the mm tree? > > There are a couple of minor conflicts against current 6.11-rc1 which > you'd best check. So please redo this against current mainline? I will rebase it.
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. 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@linux-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(-)