| Message ID | 20211025083315.4752-4-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | extend task comm from 16 to 24 | expand |
On Mon, Oct 25, 2021 at 08:33:06AM +0000, Yafang Shao wrote: > 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(). I wonder, looking at this again, if it might make more sense to avoid this cn_proc.c change, and instead, adjust get_task_comm() like so: #define get_task_comm(buf, tsk) __get_task_comm(buf, __must_be_array(buf) + sizeof(buf), tsk) This would still enforce the original goal of making sure get_task_comm() is being used on a char array, and now that __get_task_comm() will truncate & pad, it's safe to use on both too-small and too-big arrays.
On Tue, Oct 26, 2021 at 5:14 AM Kees Cook <keescook@chromium.org> wrote: > > On Mon, Oct 25, 2021 at 08:33:06AM +0000, Yafang Shao wrote: > > 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(). > > I wonder, looking at this again, if it might make more sense to avoid > this cn_proc.c change, and instead, adjust get_task_comm() like so: > > #define get_task_comm(buf, tsk) > __get_task_comm(buf, __must_be_array(buf) + sizeof(buf), tsk) > > This would still enforce the original goal of making sure > get_task_comm() is being used on a char array, and now that > __get_task_comm() will truncate & pad, it's safe to use on both > too-small and too-big arrays. > It Makes sense to me. I will do it as you suggested.
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 */
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(). __get_task_comm() always get a nul terminated string, so we don't worry about whether it is truncated or not. 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: Vladimir Zapolskiy <vzapolskiy@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: David Howells <dhowells@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: Kees Cook <keescook@chromium.org> Cc: Petr Mladek <pmladek@suse.com> --- drivers/connector/cn_proc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)