From: Kees Cook <keescook@chromium.org>
To: Yafang Shao <laoar.shao@gmail.com>
Cc: akpm@linux-foundation.org, pmladek@suse.com,
peterz@infradead.org, valentin.schneider@arm.com,
mathieu.desnoyers@efficios.com, qiang.zhang@windriver.com,
robdclark@chromium.org, viro@zeniv.linux.org.uk,
christian@brauner.io, dietmar.eggemann@arm.com,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN
Date: Wed, 29 Sep 2021 11:09:12 -0700 [thread overview]
Message-ID: <202109291108.789FCCF0DE@keescook> (raw)
In-Reply-To: <20210929115036.4851-2-laoar.shao@gmail.com>
On Wed, Sep 29, 2021 at 11:50:32AM +0000, Yafang Shao wrote:
> We can use TASK_COMM_LEN directly instread of sizeof(task->comm).
This change makes sense if we accept performing allocation for
task->comm, but I don't think that's a good idea. I'll reply to those
patches...
> This patch also replace strlcpy with strscpy, to fix the warning by
> checkpatch -
> WARNING: Prefer strscpy over strlcpy - see: https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
I like this part of the patch, though! :)
-Kees
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> arch/ia64/kernel/mca.c | 6 +++---
> drivers/block/drbd/drbd_main.c | 2 +-
> drivers/hwtracing/stm/core.c | 2 +-
> drivers/tty/tty_audit.c | 2 +-
> fs/exec.c | 2 +-
> kernel/audit.c | 4 ++--
> kernel/auditsc.c | 4 ++--
> kernel/capability.c | 4 ++--
> kernel/futex.c | 2 +-
> kernel/sys.c | 2 +-
> kernel/trace/blktrace.c | 2 +-
> security/lsm_audit.c | 4 ++--
> security/selinux/selinuxfs.c | 2 +-
> security/yama/yama_lsm.c | 2 +-
> sound/core/oss/pcm_oss.c | 2 +-
> 15 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/arch/ia64/kernel/mca.c b/arch/ia64/kernel/mca.c
> index e628a88607bb..4ee86e91ff5e 100644
> --- a/arch/ia64/kernel/mca.c
> +++ b/arch/ia64/kernel/mca.c
> @@ -875,7 +875,7 @@ copy_reg(const u64 *fr, u64 fnat, unsigned long *tr, unsigned long *tnat)
> static void
> ia64_mca_modify_comm(const struct task_struct *previous_current)
> {
> - char *p, comm[sizeof(current->comm)];
> + char *p, comm[TASK_COMM_LEN];
> if (previous_current->pid)
> snprintf(comm, sizeof(comm), "%s %d",
> current->comm, previous_current->pid);
> @@ -889,7 +889,7 @@ ia64_mca_modify_comm(const struct task_struct *previous_current)
> current->comm, l, previous_current->comm,
> task_thread_info(previous_current)->cpu);
> }
> - memcpy(current->comm, comm, sizeof(current->comm));
> + memcpy(current->comm, comm, TASK_COMM_LEN);
> }
>
> static void
> @@ -1794,7 +1794,7 @@ format_mca_init_stack(void *mca_data, unsigned long offset,
> p->parent = p->real_parent = p->group_leader = p;
> INIT_LIST_HEAD(&p->children);
> INIT_LIST_HEAD(&p->sibling);
> - strncpy(p->comm, type, sizeof(p->comm)-1);
> + strncpy(p->comm, type, TASK_COMM_LEN-1);
> }
>
> /* Caller prevents this from being called after init */
> diff --git a/drivers/block/drbd/drbd_main.c b/drivers/block/drbd/drbd_main.c
> index 55234a558e98..a7cf062e76ab 100644
> --- a/drivers/block/drbd/drbd_main.c
> +++ b/drivers/block/drbd/drbd_main.c
> @@ -306,7 +306,7 @@ static int drbd_thread_setup(void *arg)
> unsigned long flags;
> int retval;
>
> - snprintf(current->comm, sizeof(current->comm), "drbd_%c_%s",
> + snprintf(current->comm, TASK_COMM_LEN, "drbd_%c_%s",
> thi->name[0],
> resource->name);
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 2712e699ba08..8ec0a34a9ba6 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -631,7 +631,7 @@ static ssize_t stm_char_write(struct file *file, const char __user *buf,
> * point, try to use the task name and "default" policy entries.
> */
> if (!stmf->output.nr_chans) {
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
> char *ids[] = { comm, "default", NULL };
>
> get_task_comm(comm, current);
> diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> index ca7afd7b2716..b98b1aef5f6f 100644
> --- a/drivers/tty/tty_audit.c
> +++ b/drivers/tty/tty_audit.c
> @@ -69,7 +69,7 @@ static void tty_audit_log(const char *description, dev_t dev,
>
> ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TTY);
> if (ab) {
> - char name[sizeof(current->comm)];
> + char name[TASK_COMM_LEN];
>
> audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d"
> " minor=%d comm=", description, pid, uid,
> diff --git a/fs/exec.c b/fs/exec.c
> index a098c133d8d7..021c9dc727bc 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, TASK_COMM_LEN);
> task_unlock(tsk);
> perf_event_comm(tsk, exec);
> }
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 121d37e700a6..1fbd036e77be 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1549,7 +1549,7 @@ static void audit_log_multicast(int group, const char *op, int err)
> {
> const struct cred *cred;
> struct tty_struct *tty;
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
> struct audit_buffer *ab;
>
> if (!audit_enabled)
> @@ -2192,7 +2192,7 @@ void audit_put_tty(struct tty_struct *tty)
> void audit_log_task_info(struct audit_buffer *ab)
> {
> const struct cred *cred;
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
> struct tty_struct *tty;
>
> if (!ab)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8dd73a64f921..295376d7d926 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2595,7 +2595,7 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
> enum audit_nfcfgop op, gfp_t gfp)
> {
> struct audit_buffer *ab;
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
>
> ab = audit_log_start(audit_context(), gfp, AUDIT_NETFILTER_CFG);
> if (!ab)
> @@ -2616,7 +2616,7 @@ static void audit_log_task(struct audit_buffer *ab)
> kuid_t auid, uid;
> kgid_t gid;
> unsigned int sessionid;
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
>
> auid = audit_get_loginuid(current);
> sessionid = audit_get_sessionid(current);
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 46a361dde042..4033ee837f63 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -45,7 +45,7 @@ __setup("no_file_caps", file_caps_disable);
>
> static void warn_legacy_capability_use(void)
> {
> - char name[sizeof(current->comm)];
> + char name[TASK_COMM_LEN];
>
> pr_info_once("warning: `%s' uses 32-bit capabilities (legacy support in use)\n",
> get_task_comm(name, current));
> @@ -69,7 +69,7 @@ static void warn_legacy_capability_use(void)
>
> static void warn_deprecated_v2(void)
> {
> - char name[sizeof(current->comm)];
> + char name[TASK_COMM_LEN];
>
> pr_info_once("warning: `%s' uses deprecated v2 capabilities in a way that may be insecure\n",
> get_task_comm(name, current));
> diff --git a/kernel/futex.c b/kernel/futex.c
> index c15ad276fd15..6cd08156611e 100644
> --- a/kernel/futex.c
> +++ b/kernel/futex.c
> @@ -1703,7 +1703,7 @@ static int futex_atomic_op_inuser(unsigned int encoded_op, u32 __user *uaddr)
>
> if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) {
> if (oparg < 0 || oparg > 31) {
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
> /*
> * kill this print and return -EINVAL when userspace
> * is sane again
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 8fdac0d90504..bea1120c5579 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2265,7 +2265,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> unsigned long, arg4, unsigned long, arg5)
> {
> struct task_struct *me = current;
> - unsigned char comm[sizeof(me->comm)];
> + unsigned char comm[TASK_COMM_LEN];
> long error;
>
> error = security_task_prctl(option, arg2, arg3, arg4, arg5);
> diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
> index c221e4c3f625..009f77ebda5a 100644
> --- a/kernel/trace/blktrace.c
> +++ b/kernel/trace/blktrace.c
> @@ -124,7 +124,7 @@ static void trace_note_tsk(struct task_struct *tsk)
> spin_lock_irqsave(&running_trace_lock, flags);
> list_for_each_entry(bt, &running_trace_list, running_list) {
> trace_note(bt, tsk->pid, BLK_TN_PROCESS, tsk->comm,
> - sizeof(tsk->comm), 0);
> + TASK_COMM_LEN, 0);
> }
> spin_unlock_irqrestore(&running_trace_lock, flags);
> }
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 5a5016ef43b0..eec54cb48845 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -208,7 +208,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
> static void dump_common_audit_data(struct audit_buffer *ab,
> struct common_audit_data *a)
> {
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
>
> /*
> * To keep stack sizes in check force programers to notice if they
> @@ -310,7 +310,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> if (tsk) {
> pid_t pid = task_tgid_nr(tsk);
> if (pid) {
> - char comm[sizeof(tsk->comm)];
> + char comm[TASK_COMM_LEN];
> audit_log_format(ab, " opid=%d ocomm=", pid);
> audit_log_untrustedstring(ab,
> memcpy(comm, tsk->comm, sizeof(comm)));
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index e4cd7cb856f3..e529db76502d 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -752,7 +752,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
> goto out;
>
> if (new_value) {
> - char comm[sizeof(current->comm)];
> + char comm[TASK_COMM_LEN];
>
> memcpy(comm, current->comm, sizeof(comm));
> pr_warn_once("SELinux: %s (%d) set checkreqprot to 1. This is deprecated and will be rejected in a future kernel release.\n",
> diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
> index 06e226166aab..178d7a138e98 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -75,7 +75,7 @@ static void report_access(const char *access, struct task_struct *target,
> struct task_struct *agent)
> {
> struct access_report_info *info;
> - char agent_comm[sizeof(agent->comm)];
> + char agent_comm[TASK_COMM_LEN];
>
> assert_spin_locked(&target->alloc_lock); /* for target->comm */
>
> diff --git a/sound/core/oss/pcm_oss.c b/sound/core/oss/pcm_oss.c
> index 82a818734a5f..d541530236e1 100644
> --- a/sound/core/oss/pcm_oss.c
> +++ b/sound/core/oss/pcm_oss.c
> @@ -2460,7 +2460,7 @@ static int snd_task_name(struct task_struct *task, char *name, size_t size)
>
> if (snd_BUG_ON(!task || !name || size < 2))
> return -EINVAL;
> - for (idx = 0; idx < sizeof(task->comm) && idx + 1 < size; idx++)
> + for (idx = 0; idx < TASK_COMM_LEN && idx + 1 < size; idx++)
> name[idx] = task->comm[idx];
> name[idx] = '\0';
> return 0;
> --
> 2.17.1
>
--
Kees Cook
next prev parent reply other threads:[~2021-09-29 18:09 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-29 11:50 [PATCH 0/5] kthread: increase the size of kthread's comm Yafang Shao
2021-09-29 11:50 ` [PATCH 1/5] kernel: replace sizeof(task->comm) with TASK_COMM_LEN Yafang Shao
2021-09-29 18:09 ` Kees Cook [this message]
2021-09-30 12:27 ` Yafang Shao
2021-10-03 3:31 ` Al Viro
2021-10-03 14:14 ` Yafang Shao
2021-09-29 11:50 ` [PATCH 2/5] kernel/fork: allocate task->comm dynamicly Yafang Shao
2021-09-29 13:08 ` Yafang Shao
2021-09-29 18:11 ` Kees Cook
2021-09-30 12:41 ` Yafang Shao
2021-09-30 14:51 ` Petr Mladek
2021-10-01 11:58 ` Yafang Shao
2021-09-29 11:50 ` [PATCH 3/5] kernel/sched: improve the BUILD_BUG_ON() in get_task_comm() Yafang Shao
2021-09-29 18:12 ` Kees Cook
2021-09-30 12:43 ` Yafang Shao
2021-09-29 11:50 ` [PATCH 4/5] kernel: increase the size of kthread's comm Yafang Shao
2021-09-29 18:19 ` Kees Cook
2021-09-30 12:53 ` Yafang Shao
2021-09-29 11:50 ` [PATCH 5/5] kernel/kthread: show a warning if kthread's comm is still trucated Yafang Shao
2021-09-29 18:20 ` Kees Cook
2021-09-30 12:54 ` Yafang Shao
2021-09-30 15:17 ` Petr Mladek
2021-10-01 11:59 ` Yafang Shao
2021-10-03 3:41 ` [PATCH 0/5] kthread: increase the size of kthread's comm Al Viro
2021-10-03 14:20 ` Yafang Shao
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202109291108.789FCCF0DE@keescook \
--to=keescook@chromium.org \
--cc=akpm@linux-foundation.org \
--cc=christian@brauner.io \
--cc=dietmar.eggemann@arm.com \
--cc=laoar.shao@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=qiang.zhang@windriver.com \
--cc=robdclark@chromium.org \
--cc=valentin.schneider@arm.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.