This patchset changes files among many subsystems. I don't know which
tree it should be applied to, so I just base it on Linus's tree.
When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,
jbd2/nvme0n1p2-
xfs-reclaim/sdf
We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:
rcu_tasks_kthre
rcu_tasks_rude_
rcu_tasks_trace
poll_mpt3sas0_s
ext4-rsv-conver
xfs-reclaim/sd{a, b, c, ...}
xfs-blockgc/sd{a, b, c, ...}
xfs-inodegc/sd{a, b, c, ...}
audit_send_repl
ecryptfs-kthrea
vfio-irqfd-clea
jbd2/nvme0n1p2-
...
We should improve this problem fundamentally.
This patch extends the size of task comm to 24 bytes, which is the
same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.
After this patchset, the truncated kthreads listed above will be
displayed as:
rcu_tasks_kthread
rcu_tasks_rude_kthread
rcu_tasks_trace_kthread
poll_mpt3sas0_statu
ext4-rsv-conversion
xfs-reclaim/sdf1
xfs-blockgc/sdf1
xfs-inodegc/sdf1
audit_send_reply
ecryptfs-kthread
vfio-irqfd-cleanup
jbd2/nvme0n1p2-8
If the kthread's comm is still truncated, a warning will be printed.
Below is the result of my test case:
truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters
Changes since v3:
- fixes -Wstringop-truncation warning reported by kernel test robot
Changes since v2:
- avoid change UAPI code per Kees
- remove the description of out of tree code from commit log per Peter
Changes since v1:
- extend task comm to 24bytes, per Petr
- improve the warning per Petr
- make the checkpatch warning a separate patch
Yafang Shao (5):
elfcore: use TASK_COMM_LEN instead of 16 in prpsinfo
connector: use __get_task_comm in proc_comm_connector
fs/exec: use strscpy instead of strlcpy in __set_task_comm
sched.h: extend task comm from 16 to 24 for CONFIG_BASE_FULL
kernel/kthread: show a warning if kthread's comm is truncated
drivers/connector/cn_proc.c | 5 ++++-
fs/exec.c | 2 +-
include/linux/elfcore-compat.h | 2 +-
include/linux/elfcore.h | 4 ++--
include/linux/sched.h | 4 ++++
kernel/kthread.c | 7 ++++++-
6 files changed, 18 insertions(+), 6 deletions(-)
--
2.17.1
connector comm was introduced in commit
f786ecba4158 ("connector: add comm change event report to proc connector").
struct comm_proc_event was defined in include/linux/cn_proc.h first and
then been moved into file include/uapi/linux/cn_proc.h in commit
607ca46e97a1 ("UAPI: (Scripted) Disintegrate include/linux").
As this is the UAPI code, we can't change it without potentially breaking
things (i.e. userspace binaries have this size built in, so we can't just
change the size). To prepare for the followup change - extending task
comm, we have to use __get_task_comm() to avoid the BUILD_BUG_ON() in
proc_comm_connector().
Suggested-by: Kees Cook <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Vladimir Zapolskiy <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: David Howells <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
drivers/connector/cn_proc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
index 646ad385e490..c88ba2dc1eae 100644
--- a/drivers/connector/cn_proc.c
+++ b/drivers/connector/cn_proc.c
@@ -230,7 +230,10 @@ void proc_comm_connector(struct task_struct *task)
ev->what = PROC_EVENT_COMM;
ev->event_data.comm.process_pid = task->pid;
ev->event_data.comm.process_tgid = task->tgid;
- get_task_comm(ev->event_data.comm.comm, task);
+
+ /* This may get truncated. */
+ __get_task_comm(ev->event_data.comm.comm,
+ sizeof(ev->event_data.comm.comm), task);
memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
msg->ack = 0; /* not used */
--
2.17.1
Fix a warning by checkpatch -
WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
Signed-off-by: Yafang Shao <[email protected]>
Acked-by: Kees Cook <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
fs/exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index a098c133d8d7..de804c566200 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
{
task_lock(tsk);
trace_task_rename(tsk, buf);
- strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ strscpy(tsk->comm, buf, sizeof(tsk->comm));
task_unlock(tsk);
perf_event_comm(tsk, exec);
}
--
2.17.1
When I was implementing a new per-cpu kthread cfs_migration, I found the
comm of it "cfs_migration/%u" is truncated due to the limitation of
TASK_COMM_LEN. For example, the comm of the percpu thread on CPU10~19 are
all with the same name "cfs_migration/1", which will confuse the user. This
issue is not critical, because we can get the corresponding CPU from the
task's Cpus_allowed. But for kthreads correspoinding to other hardware
devices, it is not easy to get the detailed device info from task comm,
for example,
jbd2/nvme0n1p2-
xfs-reclaim/sdf
We can also shorten the name to work around this problem, but I find
there are so many truncated kthreads:
rcu_tasks_kthre
rcu_tasks_rude_
rcu_tasks_trace
poll_mpt3sas0_s
ext4-rsv-conver
xfs-reclaim/sd{a, b, c, ...}
xfs-blockgc/sd{a, b, c, ...}
xfs-inodegc/sd{a, b, c, ...}
audit_send_repl
ecryptfs-kthrea
vfio-irqfd-clea
jbd2/nvme0n1p2-
...
We should improve this problem fundamentally.
This patch extends the size of task comm to 24 bytes, which is the
same length with workqueue's, for the CONFIG_BASE_FULL case. And for the
CONFIG_BASE_SMALL case, the size of task comm is still kept as 16 bytes.
Suggested-by: Petr Mladek <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
include/linux/sched.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index c1a927ddec64..d3f947ef7d0a 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -275,7 +275,11 @@ struct task_group;
#define get_current_state() READ_ONCE(current->__state)
/* Task command name length: */
+#if CONFIG_BASE_SMALL
#define TASK_COMM_LEN 16
+#else
+#define TASK_COMM_LEN 24
+#endif
extern void scheduler_tick(void);
--
2.17.1
kernel test robot reported a -Wstringop-truncation warning after I
extend task comm from 16 to 24. Below is the detailed warning:
fs/binfmt_elf.c: In function 'fill_psinfo.isra':
>> fs/binfmt_elf.c:1575:9: warning: 'strncpy' output may be truncated copying 16 bytes from a string of length 23 [-Wstringop-truncation]
1575 | strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This patch can fix this warning.
struct elf_prpsinfo was moved from include/uapi/linux/elfcore.h into
include/linux/elfcore.h in commit
1e6b57d6421f ("unexport linux/elfcore.h")
As it is not UAPI code, we can replace 16 with TASK_COMM_LEN without
worrying about breaking userspace things.
struct elf_prpsinfo is used to dump the task information in userspace
coredump or kernel vmcore. So I verified what will happen to vmcore if
I extend the size of TASK_COMM_LEN to 24. The result is that the vmcore
still work fine as expected, for example:
crash> ps
PID PPID CPU TASK ST %MEM VSZ RSS COMM
> 0 0 0 ffffffff8501a940 RU 0.0 0 0 [swapper/0]
> 0 0 1 ffff996e00f81f80 RU 0.0 0 0 [swapper/1]
> 0 0 2 ffff996e00f80000 RU 0.0 0 0 [swapper/2]
> 0 0 3 ffff996e00f85e80 RU 0.0 0 0 [swapper/3]
> 0 0 4 ffff996e00f83f00 RU 0.0 0 0 [swapper/4]
0 0 5 ffff996e00f8de80 RU 0.0 0 0 [swapper/5]
> 0 0 6 ffff996e00f8bf00 RU 0.0 0 0 [swapper/6]
> 0 0 7 ffff996e00f89f80 RU 0.0 0 0 [swapper/7]
> 0 0 8 ffff996e00f88000 RU 0.0 0 0 [swapper/8]
> 0 0 9 ffff996e00f93f00 RU 0.0 0 0 [swapper/9]
> 0 0 10 ffff996e00f91f80 RU 0.0 0 0 [swapper/10]
> 0 0 11 ffff996e00f90000 RU 0.0 0 0 [swapper/11]
> 0 0 12 ffff996e00f95e80 RU 0.0 0 0 [swapper/12]
> 0 0 13 ffff996e00f98000 RU 0.0 0 0 [swapper/13]
> 0 0 14 ffff996e00f9de80 RU 0.0 0 0 [swapper/14]
> 0 0 15 ffff996e00f9bf00 RU 0.0 0 0 [swapper/15]
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
include/linux/elfcore-compat.h | 2 +-
include/linux/elfcore.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
index e272c3d452ce..8a52a782161d 100644
--- a/include/linux/elfcore-compat.h
+++ b/include/linux/elfcore-compat.h
@@ -43,7 +43,7 @@ struct compat_elf_prpsinfo
__compat_uid_t pr_uid;
__compat_gid_t pr_gid;
compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
- char pr_fname[16];
+ char pr_fname[TASK_COMM_LEN];
char pr_psargs[ELF_PRARGSZ];
};
diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
index 2aaa15779d50..ff4e4e455160 100644
--- a/include/linux/elfcore.h
+++ b/include/linux/elfcore.h
@@ -65,8 +65,8 @@ struct elf_prpsinfo
__kernel_gid_t pr_gid;
pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
/* Lots missing */
- char pr_fname[16]; /* filename of executable */
- char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
+ char pr_fname[TASK_COMM_LEN]; /* filename of executable */
+ char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
};
static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
--
2.17.1
Show a warning if task comm is truncated. Below is the result
of my test case:
truncated kthread comm:I-am-a-kthread-with-lon, pid:14 by 6 characters
Suggested-by: Petr Mladek <[email protected]>
Signed-off-by: Yafang Shao <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Petr Mladek <[email protected]>
---
kernel/kthread.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 5b37a8567168..46b924c92078 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -399,12 +399,17 @@ struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
if (!IS_ERR(task)) {
static const struct sched_param param = { .sched_priority = 0 };
char name[TASK_COMM_LEN];
+ int len;
/*
* task is already visible to other tasks, so updating
* COMM must be protected.
*/
- vsnprintf(name, sizeof(name), namefmt, args);
+ len = vsnprintf(name, sizeof(name), namefmt, args);
+ if (len >= TASK_COMM_LEN) {
+ pr_warn("truncated kthread comm:%s, pid:%d by %d characters\n",
+ name, task->pid, len - TASK_COMM_LEN + 1);
+ }
set_task_comm(task, name);
/*
* root may have changed our (kthreadd's) priority or CPU mask.
--
2.17.1
On Wed, 13 Oct 2021 10:23:42 +0000
Yafang Shao <[email protected]> wrote:
> kernel test robot reported a -Wstringop-truncation warning after I
> extend task comm from 16 to 24. Below is the detailed warning:
>
> fs/binfmt_elf.c: In function 'fill_psinfo.isra':
> >> fs/binfmt_elf.c:1575:9: warning: 'strncpy' output may be truncated copying 16 bytes from a string of length 23 [-Wstringop-truncation]
> 1575 | strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This patch can fix this warning.
>
> struct elf_prpsinfo was moved from include/uapi/linux/elfcore.h into
> include/linux/elfcore.h in commit
> 1e6b57d6421f ("unexport linux/elfcore.h")
>
> As it is not UAPI code, we can replace 16 with TASK_COMM_LEN without
> worrying about breaking userspace things.
>
> struct elf_prpsinfo is used to dump the task information in userspace
> coredump or kernel vmcore. So I verified what will happen to vmcore if
> I extend the size of TASK_COMM_LEN to 24. The result is that the vmcore
> still work fine as expected, for example:
>
> crash> ps
> PID PPID CPU TASK ST %MEM VSZ RSS COMM
> > 0 0 0 ffffffff8501a940 RU 0.0 0 0 [swapper/0]
> > 0 0 1 ffff996e00f81f80 RU 0.0 0 0 [swapper/1]
> > 0 0 2 ffff996e00f80000 RU 0.0 0 0 [swapper/2]
> > 0 0 3 ffff996e00f85e80 RU 0.0 0 0 [swapper/3]
> > 0 0 4 ffff996e00f83f00 RU 0.0 0 0 [swapper/4]
> 0 0 5 ffff996e00f8de80 RU 0.0 0 0 [swapper/5]
> > 0 0 6 ffff996e00f8bf00 RU 0.0 0 0 [swapper/6]
> > 0 0 7 ffff996e00f89f80 RU 0.0 0 0 [swapper/7]
> > 0 0 8 ffff996e00f88000 RU 0.0 0 0 [swapper/8]
> > 0 0 9 ffff996e00f93f00 RU 0.0 0 0 [swapper/9]
> > 0 0 10 ffff996e00f91f80 RU 0.0 0 0 [swapper/10]
> > 0 0 11 ffff996e00f90000 RU 0.0 0 0 [swapper/11]
> > 0 0 12 ffff996e00f95e80 RU 0.0 0 0 [swapper/12]
> > 0 0 13 ffff996e00f98000 RU 0.0 0 0 [swapper/13]
> > 0 0 14 ffff996e00f9de80 RU 0.0 0 0 [swapper/14]
> > 0 0 15 ffff996e00f9bf00 RU 0.0 0 0 [swapper/15]
>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Yafang Shao <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> include/linux/elfcore-compat.h | 2 +-
> include/linux/elfcore.h | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
> index e272c3d452ce..8a52a782161d 100644
> --- a/include/linux/elfcore-compat.h
> +++ b/include/linux/elfcore-compat.h
> @@ -43,7 +43,7 @@ struct compat_elf_prpsinfo
> __compat_uid_t pr_uid;
> __compat_gid_t pr_gid;
> compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
> - char pr_fname[16];
> + char pr_fname[TASK_COMM_LEN];
> char pr_psargs[ELF_PRARGSZ];
> };
Nice clean up, but should we add "#include <linux/sched.h>" to this header,
to make sure that it pulls in TASK_COMM_LEN define and not just hope it
gets pulled in beforehand by chance?
>
> diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
> index 2aaa15779d50..ff4e4e455160 100644
> --- a/include/linux/elfcore.h
> +++ b/include/linux/elfcore.h
> @@ -65,8 +65,8 @@ struct elf_prpsinfo
> __kernel_gid_t pr_gid;
> pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
> /* Lots missing */
> - char pr_fname[16]; /* filename of executable */
> - char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
> + char pr_fname[TASK_COMM_LEN]; /* filename of executable */
> + char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
This header is fine, as it pulls in sched/task_stack.h which includes
sched.h.
-- Steve
> };
>
> static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
On Wed, 13 Oct 2021 10:23:43 +0000
Yafang Shao <[email protected]> wrote:
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c
> @@ -230,7 +230,10 @@ void proc_comm_connector(struct task_struct *task)
> ev->what = PROC_EVENT_COMM;
> ev->event_data.comm.process_pid = task->pid;
> ev->event_data.comm.process_tgid = task->tgid;
> - get_task_comm(ev->event_data.comm.comm, task);
> +
> + /* This may get truncated. */
> + __get_task_comm(ev->event_data.comm.comm,
> + sizeof(ev->event_data.comm.comm), task);
>
> memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
> msg->ack = 0; /* not used */
__get_task_comm() uses strncpy() which my understanding is, does not add
the nul terminating byte when truncating. Which changes the functionality
here. As all task comms have a terminating byte, the old method would copy
that and include it. This won't add the terminating byte if the buffer is
smaller than the comm, and that might cause issues.
-- Steve
On Wed, Oct 13, 2021 at 9:11 PM Steven Rostedt <[email protected]> wrote:
>
> On Wed, 13 Oct 2021 10:23:42 +0000
> Yafang Shao <[email protected]> wrote:
>
> > kernel test robot reported a -Wstringop-truncation warning after I
> > extend task comm from 16 to 24. Below is the detailed warning:
> >
> > fs/binfmt_elf.c: In function 'fill_psinfo.isra':
> > >> fs/binfmt_elf.c:1575:9: warning: 'strncpy' output may be truncated copying 16 bytes from a string of length 23 [-Wstringop-truncation]
> > 1575 | strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname));
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > This patch can fix this warning.
> >
> > struct elf_prpsinfo was moved from include/uapi/linux/elfcore.h into
> > include/linux/elfcore.h in commit
> > 1e6b57d6421f ("unexport linux/elfcore.h")
> >
> > As it is not UAPI code, we can replace 16 with TASK_COMM_LEN without
> > worrying about breaking userspace things.
> >
> > struct elf_prpsinfo is used to dump the task information in userspace
> > coredump or kernel vmcore. So I verified what will happen to vmcore if
> > I extend the size of TASK_COMM_LEN to 24. The result is that the vmcore
> > still work fine as expected, for example:
> >
> > crash> ps
> > PID PPID CPU TASK ST %MEM VSZ RSS COMM
> > > 0 0 0 ffffffff8501a940 RU 0.0 0 0 [swapper/0]
> > > 0 0 1 ffff996e00f81f80 RU 0.0 0 0 [swapper/1]
> > > 0 0 2 ffff996e00f80000 RU 0.0 0 0 [swapper/2]
> > > 0 0 3 ffff996e00f85e80 RU 0.0 0 0 [swapper/3]
> > > 0 0 4 ffff996e00f83f00 RU 0.0 0 0 [swapper/4]
> > 0 0 5 ffff996e00f8de80 RU 0.0 0 0 [swapper/5]
> > > 0 0 6 ffff996e00f8bf00 RU 0.0 0 0 [swapper/6]
> > > 0 0 7 ffff996e00f89f80 RU 0.0 0 0 [swapper/7]
> > > 0 0 8 ffff996e00f88000 RU 0.0 0 0 [swapper/8]
> > > 0 0 9 ffff996e00f93f00 RU 0.0 0 0 [swapper/9]
> > > 0 0 10 ffff996e00f91f80 RU 0.0 0 0 [swapper/10]
> > > 0 0 11 ffff996e00f90000 RU 0.0 0 0 [swapper/11]
> > > 0 0 12 ffff996e00f95e80 RU 0.0 0 0 [swapper/12]
> > > 0 0 13 ffff996e00f98000 RU 0.0 0 0 [swapper/13]
> > > 0 0 14 ffff996e00f9de80 RU 0.0 0 0 [swapper/14]
> > > 0 0 15 ffff996e00f9bf00 RU 0.0 0 0 [swapper/15]
> >
> > Reported-by: kernel test robot <[email protected]>
> > Signed-off-by: Yafang Shao <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > ---
> > include/linux/elfcore-compat.h | 2 +-
> > include/linux/elfcore.h | 4 ++--
> > 2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/elfcore-compat.h b/include/linux/elfcore-compat.h
> > index e272c3d452ce..8a52a782161d 100644
> > --- a/include/linux/elfcore-compat.h
> > +++ b/include/linux/elfcore-compat.h
> > @@ -43,7 +43,7 @@ struct compat_elf_prpsinfo
> > __compat_uid_t pr_uid;
> > __compat_gid_t pr_gid;
> > compat_pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
> > - char pr_fname[16];
> > + char pr_fname[TASK_COMM_LEN];
> > char pr_psargs[ELF_PRARGSZ];
> > };
>
> Nice clean up, but should we add "#include <linux/sched.h>" to this header,
> to make sure that it pulls in TASK_COMM_LEN define and not just hope it
> gets pulled in beforehand by chance?
>
Sure, I will add it.
Thanks for the suggestion.
> >
> > diff --git a/include/linux/elfcore.h b/include/linux/elfcore.h
> > index 2aaa15779d50..ff4e4e455160 100644
> > --- a/include/linux/elfcore.h
> > +++ b/include/linux/elfcore.h
> > @@ -65,8 +65,8 @@ struct elf_prpsinfo
> > __kernel_gid_t pr_gid;
> > pid_t pr_pid, pr_ppid, pr_pgrp, pr_sid;
> > /* Lots missing */
> > - char pr_fname[16]; /* filename of executable */
> > - char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
> > + char pr_fname[TASK_COMM_LEN]; /* filename of executable */
> > + char pr_psargs[ELF_PRARGSZ]; /* initial part of arg list */
>
> This header is fine, as it pulls in sched/task_stack.h which includes
> sched.h.
>
> -- Steve
>
> > };
> >
> > static inline void elf_core_copy_regs(elf_gregset_t *elfregs, struct pt_regs *regs)
>
--
Thanks
Yafang
On Wed, Oct 13, 2021 at 10:19 PM Steven Rostedt <[email protected]> wrote:
>
> On Wed, 13 Oct 2021 10:23:43 +0000
> Yafang Shao <[email protected]> wrote:
>
> > --- a/drivers/connector/cn_proc.c
> > +++ b/drivers/connector/cn_proc.c
> > @@ -230,7 +230,10 @@ void proc_comm_connector(struct task_struct *task)
> > ev->what = PROC_EVENT_COMM;
> > ev->event_data.comm.process_pid = task->pid;
> > ev->event_data.comm.process_tgid = task->tgid;
> > - get_task_comm(ev->event_data.comm.comm, task);
> > +
> > + /* This may get truncated. */
> > + __get_task_comm(ev->event_data.comm.comm,
> > + sizeof(ev->event_data.comm.comm), task);
> >
> > memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id));
> > msg->ack = 0; /* not used */
>
> __get_task_comm() uses strncpy() which my understanding is, does not add
> the nul terminating byte when truncating. Which changes the functionality
> here. As all task comms have a terminating byte, the old method would copy
> that and include it. This won't add the terminating byte if the buffer is
> smaller than the comm, and that might cause issues.
>
Right, that is a problem.
It seems that we should add a new helper get_task_comm_may_truncated().
--
Thanks
Yafang
On Thu, 14 Oct 2021 09:48:09 +0800
Yafang Shao <[email protected]> wrote:
> > __get_task_comm() uses strncpy() which my understanding is, does not add
> > the nul terminating byte when truncating. Which changes the functionality
> > here. As all task comms have a terminating byte, the old method would copy
> > that and include it. This won't add the terminating byte if the buffer is
> > smaller than the comm, and that might cause issues.
> >
>
> Right, that is a problem.
> It seems that we should add a new helper get_task_comm_may_truncated().
Or simply change __get_task_comm() to:
char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
{
task_lock(tsk);
strncpy(buf, tsk->comm, buf_size);
/* The copied value is always nul terminated */
buf[buf_size - 1] = '\0';
task_unlock(tsk);
return buf;
}
But that should probably be a separate patch.
-- Steve
On Thu, Oct 14, 2021 at 10:24 AM Steven Rostedt <[email protected]> wrote:
>
> On Thu, 14 Oct 2021 09:48:09 +0800
> Yafang Shao <[email protected]> wrote:
>
> > > __get_task_comm() uses strncpy() which my understanding is, does not add
> > > the nul terminating byte when truncating. Which changes the functionality
> > > here. As all task comms have a terminating byte, the old method would copy
> > > that and include it. This won't add the terminating byte if the buffer is
> > > smaller than the comm, and that might cause issues.
> > >
> >
> > Right, that is a problem.
> > It seems that we should add a new helper get_task_comm_may_truncated().
>
> Or simply change __get_task_comm() to:
>
> char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> {
> task_lock(tsk);
> strncpy(buf, tsk->comm, buf_size);
> /* The copied value is always nul terminated */
> buf[buf_size - 1] = '\0';
> task_unlock(tsk);
> return buf;
> }
>
That is better! Thanks for the suggestion.
> But that should probably be a separate patch.
>
Sure.
--
Thanks
Yafang
On Wed, Oct 13, 2021 at 10:24:18PM -0400, Steven Rostedt wrote:
> On Thu, 14 Oct 2021 09:48:09 +0800
> Yafang Shao <[email protected]> wrote:
>
> > > __get_task_comm() uses strncpy() which my understanding is, does not add
> > > the nul terminating byte when truncating. Which changes the functionality
> > > here. As all task comms have a terminating byte, the old method would copy
> > > that and include it. This won't add the terminating byte if the buffer is
> > > smaller than the comm, and that might cause issues.
> > >
> >
> > Right, that is a problem.
> > It seems that we should add a new helper get_task_comm_may_truncated().
>
> Or simply change __get_task_comm() to:
>
> char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> {
> task_lock(tsk);
> strncpy(buf, tsk->comm, buf_size);
> /* The copied value is always nul terminated */
> buf[buf_size - 1] = '\0';
> task_unlock(tsk);
> return buf;
> }
>
> But that should probably be a separate patch.
strscpy_pad() is the right thing here -- it'll retain the NUL-fill
properties of strncpy and terminate correctly.
The use of non-terminating issue with strncpy() wasn't a problem here
because get_task_comm() would always make sure task->comm was
terminated. (It uses strlcpy(), which I think needs to be changed to
strscpy_pad() too...)
--
Kees Cook
On Thu, Oct 14, 2021 at 12:50 PM Kees Cook <[email protected]> wrote:
>
> On Wed, Oct 13, 2021 at 10:24:18PM -0400, Steven Rostedt wrote:
> > On Thu, 14 Oct 2021 09:48:09 +0800
> > Yafang Shao <[email protected]> wrote:
> >
> > > > __get_task_comm() uses strncpy() which my understanding is, does not add
> > > > the nul terminating byte when truncating. Which changes the functionality
> > > > here. As all task comms have a terminating byte, the old method would copy
> > > > that and include it. This won't add the terminating byte if the buffer is
> > > > smaller than the comm, and that might cause issues.
> > > >
> > >
> > > Right, that is a problem.
> > > It seems that we should add a new helper get_task_comm_may_truncated().
> >
> > Or simply change __get_task_comm() to:
> >
> > char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> > {
> > task_lock(tsk);
> > strncpy(buf, tsk->comm, buf_size);
> > /* The copied value is always nul terminated */
> > buf[buf_size - 1] = '\0';
> > task_unlock(tsk);
> > return buf;
> > }
> >
> > But that should probably be a separate patch.
>
> strscpy_pad() is the right thing here -- it'll retain the NUL-fill
> properties of strncpy and terminate correctly.
>
strscpy_pad() can also work, and seems more simple.
> The use of non-terminating issue with strncpy() wasn't a problem here
> because get_task_comm() would always make sure task->comm was
> terminated. (It uses strlcpy(), which I think needs to be changed to
> strscpy_pad() too...)
>
> --
> Kees Cook
--
Thanks
Yafang
On Thu, 14 Oct 2021 17:26:16 +0800
Yafang Shao <[email protected]> wrote:
> > > But that should probably be a separate patch.
> >
> > strscpy_pad() is the right thing here -- it'll retain the NUL-fill
> > properties of strncpy and terminate correctly.
> >
>
> strscpy_pad() can also work, and seems more simple.
I'm fine either way. As long as the string that is updated is terminated.
-- Steve