[PATCH v8 0/8] Improve the copy of task comm

[PATCH v8 0/8] Improve the copy of task comm

[Yafang Shao]

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~#8:   strcpy

Please note that strncpy() is not included in this series as it is being
tracked by another effort. [1]

In this series, we have removed __get_task_comm() because the task_lock()
and BUILD_BUG_ON() within it are unnecessary.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=wjAmmHUg6vho1KjzQi2=psR30+CogFd4aXrThr2gsiS4g@mail.gmail.com/ [0]

Changes:
v7->v8:
- Avoid '+1' and '-1' in string copy. (Alejandro)

v6->v7: https://lore.kernel.org/all/20240817025624.13157-1-laoar.shao@gmail.com/
- Improve the comment (Alejandro)
- Drop strncpy as it is being tracked by another effort (Justin)
  https://github.com/KSPP/linux/issues/90 [1]

v5->v6: https://lore.kernel.org/linux-mm/20240812022933.69850-1-laoar.shao@gmail.com/
- Get rid of __get_task_comm() (Linus)
- Use ARRAY_SIZE() in get_task_comm() (Alejandro)

v4->v5: https://lore.kernel.org/all/20240804075619.20804-1-laoar.shao@gmail.com/
- Drop changes in the mm/kmemleak.c as it was fixed by
  commit 0b84780134fb ("mm/kmemleak: replace strncpy() with strscpy()")
- Drop changes in kernel/tsacct.c as it was fixed by
  commmit 0fe2356434e ("tsacct: replace strncpy() with strscpy()")

v3->v4: https://lore.kernel.org/linux-mm/20240729023719.1933-1-laoar.shao@gmail.com/
- 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 (8):
  Get rid of __get_task_comm()
  auditsc: Replace memcpy() with strscpy()
  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}
  net: Replace strcpy() with strscpy()
  drm: Replace strcpy() with strscpy()

 drivers/gpu/drm/drm_framebuffer.c     |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c |  2 +-
 fs/exec.c                             | 10 -----
 fs/proc/array.c                       |  2 +-
 include/linux/sched.h                 | 32 +++++++++++---
 kernel/auditsc.c                      |  6 +--
 kernel/kthread.c                      |  2 +-
 mm/util.c                             | 62 ++++++++++++---------------
 net/ipv6/ndisc.c                      |  2 +-
 security/lsm_audit.c                  |  4 +-
 security/selinux/selinuxfs.c          |  2 +-
 tools/bpf/bpftool/pids.c              |  2 +
 12 files changed, 66 insertions(+), 62 deletions(-)

-- 
2.43.5

[PATCH v8 1/8] Get rid of __get_task_comm()

[Yafang Shao]

We want to eliminate the use of __get_task_comm() for the following
reasons:

- The task_lock() is unnecessary
  Quoted from Linus [0]:
  : Since user space can randomly change their names anyway, using locking
  : was always wrong for readers (for writers it probably does make sense
  : to have some lock - although practically speaking nobody cares there
  : either, but at least for a writer some kind of race could have
  : long-term mixed results

- The BUILD_BUG_ON() doesn't add any value
  The only requirement is to ensure that the destination buffer is a valid
  array.

- Zeroing is not necessary in current use cases
  To avoid confusion, we should remove it. Moreover, not zeroing could
  potentially make it easier to uncover bugs. If the caller needs a
  zero-padded task name, it should be explicitly handled at the call site.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
Suggested-by: Alejandro Colomar <alx@kernel.org>
Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Matus Jokay <matus.jokay@stuba.sk>
Cc: Alejandro Colomar <alx@kernel.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
---
 fs/exec.c             | 10 ----------
 fs/proc/array.c       |  2 +-
 include/linux/sched.h | 32 ++++++++++++++++++++++++++------
 kernel/kthread.c      |  2 +-
 4 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 50e76cc633c4..8a23171bc3c3 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1264,16 +1264,6 @@ static int unshare_sighand(struct task_struct *me)
 	return 0;
 }
 
-char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
-{
-	task_lock(tsk);
-	/* Always NUL terminated and zero-padded */
-	strscpy_pad(buf, tsk->comm, buf_size);
-	task_unlock(tsk);
-	return buf;
-}
-EXPORT_SYMBOL_GPL(__get_task_comm);
-
 /*
  * These functions flushes out all traces of the currently running executable
  * so that a new one can be started
diff --git a/fs/proc/array.c b/fs/proc/array.c
index 34a47fb0c57f..55ed3510d2bb 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
 	else if (p->flags & PF_KTHREAD)
 		get_kthread_comm(tcomm, sizeof(tcomm), p);
 	else
-		__get_task_comm(tcomm, sizeof(tcomm), p);
+		get_task_comm(tcomm, p);
 
 	if (escape)
 		seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
diff --git a/include/linux/sched.h b/include/linux/sched.h
index f8d150343d42..c40b95a79d80 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1096,9 +1096,12 @@ struct task_struct {
 	/*
 	 * executable name, excluding path.
 	 *
-	 * - normally initialized setup_new_exec()
-	 * - access it with [gs]et_task_comm()
-	 * - lock it with task_lock()
+	 * - normally initialized begin_new_exec()
+	 * - set it with set_task_comm()
+	 *   - strscpy_pad() to ensure it is always NUL-terminated and
+	 *     zero-padded
+	 *   - task_lock() to ensure the operation is atomic and the name is
+	 *     fully updated.
 	 */
 	char				comm[TASK_COMM_LEN];
 
@@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
 	__set_task_comm(tsk, from, false);
 }
 
-extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
+/*
+ * - Why not use task_lock()?
+ *   User space can randomly change their names anyway, so locking for readers
+ *   doesn't make sense. For writers, locking is probably necessary, as a race
+ *   condition could lead to long-term mixed results.
+ *   The strscpy_pad() in __set_task_comm() can ensure that the task comm is
+ *   always NUL-terminated and zero-padded. Therefore the race condition between
+ *   reader and writer is not an issue.
+ *
+ * - Why not use strscpy_pad()?
+ *   While strscpy_pad() prevents writing garbage past the NUL terminator, which
+ *   is useful when using the task name as a key in a hash map, most use cases
+ *   don't require this. Zero-padding might confuse users if it’s unnecessary,
+ *   and not zeroing might even make it easier to expose bugs. If you need a
+ *   zero-padded task name, please handle that explicitly at the call site.
+ *
+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
+ */
 #define get_task_comm(buf, tsk) ({			\
-	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
-	__get_task_comm(buf, sizeof(buf), tsk);		\
+	strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));	\
+	buf;						\
 })
 
 #ifdef CONFIG_SMP
diff --git a/kernel/kthread.c b/kernel/kthread.c
index f7be976ff88a..7d001d033cf9 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
 	struct kthread *kthread = to_kthread(tsk);
 
 	if (!kthread || !kthread->full_name) {
-		__get_task_comm(buf, buf_size, tsk);
+		strscpy(buf, tsk->comm, buf_size);
 		return;
 	}
 
-- 
2.43.5

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 4410 bytes --]

Hi Yafang,

On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:
> We want to eliminate the use of __get_task_comm() for the following
> reasons:
> 
> - The task_lock() is unnecessary
>   Quoted from Linus [0]:
>   : Since user space can randomly change their names anyway, using locking
>   : was always wrong for readers (for writers it probably does make sense
>   : to have some lock - although practically speaking nobody cares there
>   : either, but at least for a writer some kind of race could have
>   : long-term mixed results
> 
> - The BUILD_BUG_ON() doesn't add any value
>   The only requirement is to ensure that the destination buffer is a valid
>   array.
> 
> - Zeroing is not necessary in current use cases
>   To avoid confusion, we should remove it. Moreover, not zeroing could
>   potentially make it easier to uncover bugs. If the caller needs a
>   zero-padded task name, it should be explicitly handled at the call site.
> 
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> Suggested-by: Alejandro Colomar <alx@kernel.org>
> Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> Cc: Matus Jokay <matus.jokay@stuba.sk>
> Cc: Alejandro Colomar <alx@kernel.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> ---
>  fs/exec.c             | 10 ----------
>  fs/proc/array.c       |  2 +-
>  include/linux/sched.h | 32 ++++++++++++++++++++++++++------
>  kernel/kthread.c      |  2 +-
>  4 files changed, 28 insertions(+), 18 deletions(-)
> 

[...]

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index f8d150343d42..c40b95a79d80 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h

[...]

> @@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
>  	__set_task_comm(tsk, from, false);
>  }
>  
> -extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> +/*

[...]

> + * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
> + */
>  #define get_task_comm(buf, tsk) ({			\
> -	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
> -	__get_task_comm(buf, sizeof(buf), tsk);		\
> +	strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));	\

I see that there's a two-argument macro

	#define strscpy(dst, src)	sized_strscpy(dst, src, sizeof(dst))

which is used in patch 2/8

	diff --git a/kernel/auditsc.c b/kernel/auditsc.c
	index 6f0d6fb6523f..e4ef5e57dde9 100644
	--- a/kernel/auditsc.c
	+++ b/kernel/auditsc.c
	@@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
		context->target_uid = task_uid(t);
		context->target_sessionid = audit_get_sessionid(t);
		security_task_getsecid_obj(t, &context->target_sid);
	-       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
	+       strscpy(context->target_comm, t->comm);
	 }

	 /**

I propose modifying that macro to use ARRAY_SIZE() instead of sizeof(),
and then calling that macro here too.  That would not only make sure
that this is an array, but make sure that every call to that macro is an
array.  An if there are macros for similar string functions that reduce
the argument with a usual sizeof(), the same thing could be done to
those too.

Have a lovley day!
Alex

> +	buf;						\
>  })
>  
>  #ifdef CONFIG_SMP
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index f7be976ff88a..7d001d033cf9 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
>  	struct kthread *kthread = to_kthread(tsk);
>  
>  	if (!kthread || !kthread->full_name) {
> -		__get_task_comm(buf, buf_size, tsk);
> +		strscpy(buf, tsk->comm, buf_size);
>  		return;
>  	}
>  
> -- 
> 2.43.5
> 

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Yafang Shao]

On Wed, Aug 28, 2024 at 6:15 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> Hi Yafang,
>
> On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:
> > We want to eliminate the use of __get_task_comm() for the following
> > reasons:
> >
> > - The task_lock() is unnecessary
> >   Quoted from Linus [0]:
> >   : Since user space can randomly change their names anyway, using locking
> >   : was always wrong for readers (for writers it probably does make sense
> >   : to have some lock - although practically speaking nobody cares there
> >   : either, but at least for a writer some kind of race could have
> >   : long-term mixed results
> >
> > - The BUILD_BUG_ON() doesn't add any value
> >   The only requirement is to ensure that the destination buffer is a valid
> >   array.
> >
> > - Zeroing is not necessary in current use cases
> >   To avoid confusion, we should remove it. Moreover, not zeroing could
> >   potentially make it easier to uncover bugs. If the caller needs a
> >   zero-padded task name, it should be explicitly handled at the call site.
> >
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> > Suggested-by: Alejandro Colomar <alx@kernel.org>
> > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Cc: Matus Jokay <matus.jokay@stuba.sk>
> > Cc: Alejandro Colomar <alx@kernel.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > ---
> >  fs/exec.c             | 10 ----------
> >  fs/proc/array.c       |  2 +-
> >  include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> >  kernel/kthread.c      |  2 +-
> >  4 files changed, 28 insertions(+), 18 deletions(-)
> >
>
> [...]
>
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index f8d150343d42..c40b95a79d80 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
>
> [...]
>
> > @@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> >       __set_task_comm(tsk, from, false);
> >  }
> >
> > -extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> > +/*
>
> [...]
>
> > + * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
> > + */
> >  #define get_task_comm(buf, tsk) ({                   \
> > -     BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
> > -     __get_task_comm(buf, sizeof(buf), tsk);         \
> > +     strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));     \
>
> I see that there's a two-argument macro
>
>         #define strscpy(dst, src)       sized_strscpy(dst, src, sizeof(dst))

This macro is defined in arch/um/include/shared/user.h, which is not
used outside
the arch/um/ directory.
This marco should be addressed.

>
> which is used in patch 2/8

The strscpy() function used in this series is defined in
include/linux/string.h, which already checks whether the input is an
array:

#define __strscpy0(dst, src, ...)       \
        sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
#define __strscpy1(dst, src, size)      sized_strscpy(dst, src, size)

#define __strscpy_pad0(dst, src, ...)   \
        sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
#define __strscpy_pad1(dst, src, size)  sized_strscpy_pad(dst, src, size)


>
>         diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>         index 6f0d6fb6523f..e4ef5e57dde9 100644
>         --- a/kernel/auditsc.c
>         +++ b/kernel/auditsc.c
>         @@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
>                 context->target_uid = task_uid(t);
>                 context->target_sessionid = audit_get_sessionid(t);
>                 security_task_getsecid_obj(t, &context->target_sid);
>         -       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
>         +       strscpy(context->target_comm, t->comm);
>          }
>
>          /**
>
> I propose modifying that macro to use ARRAY_SIZE() instead of sizeof(),
> and then calling that macro here too.  That would not only make sure
> that this is an array, but make sure that every call to that macro is an
> array.  An if there are macros for similar string functions that reduce
> the argument with a usual sizeof(), the same thing could be done to
> those too.

I have no preference between using ARRAY_SIZE() or sizeof(dst) +
__must_be_array(dst). However, for consistency, it might be better to
use ARRAY_SIZE().


--
Regards

Yafang

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 5628 bytes --]

On Wed, Aug 28, 2024 at 12:15:40PM GMT, Alejandro Colomar wrote:
> Hi Yafang,
> 
> On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:
> > We want to eliminate the use of __get_task_comm() for the following
> > reasons:
> > 
> > - The task_lock() is unnecessary
> >   Quoted from Linus [0]:
> >   : Since user space can randomly change their names anyway, using locking
> >   : was always wrong for readers (for writers it probably does make sense
> >   : to have some lock - although practically speaking nobody cares there
> >   : either, but at least for a writer some kind of race could have
> >   : long-term mixed results
> > 
> > - The BUILD_BUG_ON() doesn't add any value
> >   The only requirement is to ensure that the destination buffer is a valid
> >   array.
> > 
> > - Zeroing is not necessary in current use cases
> >   To avoid confusion, we should remove it. Moreover, not zeroing could
> >   potentially make it easier to uncover bugs. If the caller needs a
> >   zero-padded task name, it should be explicitly handled at the call site.
> > 
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> > Suggested-by: Alejandro Colomar <alx@kernel.org>
> > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > Cc: Christian Brauner <brauner@kernel.org>
> > Cc: Jan Kara <jack@suse.cz>
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Cc: Matus Jokay <matus.jokay@stuba.sk>
> > Cc: Alejandro Colomar <alx@kernel.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > ---
> >  fs/exec.c             | 10 ----------
> >  fs/proc/array.c       |  2 +-
> >  include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> >  kernel/kthread.c      |  2 +-
> >  4 files changed, 28 insertions(+), 18 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index f8d150343d42..c40b95a79d80 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> 
> [...]
> 
> > @@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> >  	__set_task_comm(tsk, from, false);
> >  }
> >  
> > -extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> > +/*
> 
> [...]
> 
> > + * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
> > + */
> >  #define get_task_comm(buf, tsk) ({			\
> > -	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
> > -	__get_task_comm(buf, sizeof(buf), tsk);		\
> > +	strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));	\
> 
> I see that there's a two-argument macro
> 
> 	#define strscpy(dst, src)	sized_strscpy(dst, src, sizeof(dst))
> 
> which is used in patch 2/8
> 
> 	diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> 	index 6f0d6fb6523f..e4ef5e57dde9 100644
> 	--- a/kernel/auditsc.c
> 	+++ b/kernel/auditsc.c
> 	@@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
> 		context->target_uid = task_uid(t);
> 		context->target_sessionid = audit_get_sessionid(t);
> 		security_task_getsecid_obj(t, &context->target_sid);
> 	-       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> 	+       strscpy(context->target_comm, t->comm);
> 	 }
> 
> 	 /**

Ahh, the actual generic definition is in <include/linux/string.h>.
You could do

	diff --git i/include/linux/string.h w/include/linux/string.h
	index 9edace076ddb..060504719904 100644
	--- i/include/linux/string.h
	+++ w/include/linux/string.h
	@@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
	  * known size.
	  */
	 #define __strscpy0(dst, src, ...)      \
	-       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
	+       sized_strscpy(dst, src, ARRAY_SIZE(dst))
	 #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
	 
	 #define __strscpy_pad0(dst, src, ...)  \
	-       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
	+       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
	 #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
	 
	 /**

> 
> I propose modifying that macro to use ARRAY_SIZE() instead of sizeof(),
> and then calling that macro here too.  That would not only make sure
> that this is an array, but make sure that every call to that macro is an
> array.  An if there are macros for similar string functions that reduce
> the argument with a usual sizeof(), the same thing could be done to
> those too.
> 
> Have a lovley day!
> Alex
> 
> > +	buf;						\
> >  })
> >  
> >  #ifdef CONFIG_SMP
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index f7be976ff88a..7d001d033cf9 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> >  	struct kthread *kthread = to_kthread(tsk);
> >  
> >  	if (!kthread || !kthread->full_name) {
> > -		__get_task_comm(buf, buf_size, tsk);
> > +		strscpy(buf, tsk->comm, buf_size);
> >  		return;
> >  	}
> >  
> > -- 
> > 2.43.5
> > 
> 
> -- 
> <https://www.alejandro-colomar.es/>



-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Yafang Shao]

On Wed, Aug 28, 2024 at 8:58 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> On Wed, Aug 28, 2024 at 12:15:40PM GMT, Alejandro Colomar wrote:
> > Hi Yafang,
> >
> > On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:
> > > We want to eliminate the use of __get_task_comm() for the following
> > > reasons:
> > >
> > > - The task_lock() is unnecessary
> > >   Quoted from Linus [0]:
> > >   : Since user space can randomly change their names anyway, using locking
> > >   : was always wrong for readers (for writers it probably does make sense
> > >   : to have some lock - although practically speaking nobody cares there
> > >   : either, but at least for a writer some kind of race could have
> > >   : long-term mixed results
> > >
> > > - The BUILD_BUG_ON() doesn't add any value
> > >   The only requirement is to ensure that the destination buffer is a valid
> > >   array.
> > >
> > > - Zeroing is not necessary in current use cases
> > >   To avoid confusion, we should remove it. Moreover, not zeroing could
> > >   potentially make it easier to uncover bugs. If the caller needs a
> > >   zero-padded task name, it should be explicitly handled at the call site.
> > >
> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> > > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> > > Suggested-by: Alejandro Colomar <alx@kernel.org>
> > > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Jan Kara <jack@suse.cz>
> > > Cc: Eric Biederman <ebiederm@xmission.com>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > > Cc: Matus Jokay <matus.jokay@stuba.sk>
> > > Cc: Alejandro Colomar <alx@kernel.org>
> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > > ---
> > >  fs/exec.c             | 10 ----------
> > >  fs/proc/array.c       |  2 +-
> > >  include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> > >  kernel/kthread.c      |  2 +-
> > >  4 files changed, 28 insertions(+), 18 deletions(-)
> > >
> >
> > [...]
> >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index f8d150343d42..c40b95a79d80 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> >
> > [...]
> >
> > > @@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> > >     __set_task_comm(tsk, from, false);
> > >  }
> > >
> > > -extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> > > +/*
> >
> > [...]
> >
> > > + * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
> > > + */
> > >  #define get_task_comm(buf, tsk) ({                 \
> > > -   BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
> > > -   __get_task_comm(buf, sizeof(buf), tsk);         \
> > > +   strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));     \
> >
> > I see that there's a two-argument macro
> >
> >       #define strscpy(dst, src)       sized_strscpy(dst, src, sizeof(dst))
> >
> > which is used in patch 2/8
> >
> >       diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >       index 6f0d6fb6523f..e4ef5e57dde9 100644
> >       --- a/kernel/auditsc.c
> >       +++ b/kernel/auditsc.c
> >       @@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
> >               context->target_uid = task_uid(t);
> >               context->target_sessionid = audit_get_sessionid(t);
> >               security_task_getsecid_obj(t, &context->target_sid);
> >       -       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> >       +       strscpy(context->target_comm, t->comm);
> >        }
> >
> >        /**
>
> Ahh, the actual generic definition is in <include/linux/string.h>.
> You could do
>
>         diff --git i/include/linux/string.h w/include/linux/string.h
>         index 9edace076ddb..060504719904 100644
>         --- i/include/linux/string.h
>         +++ w/include/linux/string.h
>         @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>           * known size.
>           */
>          #define __strscpy0(dst, src, ...)      \
>         -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
>         +       sized_strscpy(dst, src, ARRAY_SIZE(dst))
>          #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
>
>          #define __strscpy_pad0(dst, src, ...)  \
>         -       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
>         +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
>          #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
>
>          /**

Thank you for your suggestion. How does the following commit log look
to you? Does it meet your expectations?

    string: Use ARRAY_SIZE() in strscpy()

    We can use ARRAY_SIZE() instead to clarify that they are regular characters.

    Co-developed-by: Alejandro Colomar <alx@kernel.org>
    Signed-off-by: Alejandro Colomar <alx@kernel.org>
    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>

diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
index bbab79c0c074..07216996e3a9 100644
--- a/arch/um/include/shared/user.h
+++ b/arch/um/include/shared/user.h
@@ -14,7 +14,7 @@
  * copying too much infrastructure for my taste, so userspace files
  * get less checking than kernel files.
  */
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))

 /* This is to get size_t and NULL */
 #ifndef __UM_HOST__
@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
const char *prefix_str,
 extern int in_aton(char *str);
 extern size_t strlcat(char *, const char *, size_t);
 extern size_t sized_strscpy(char *, const char *, size_t);
-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))

 /* Copied from linux/compiler-gcc.h since we can't include it directly */
 #define barrier() __asm__ __volatile__("": : :"memory")
diff --git a/include/linux/string.h b/include/linux/string.h
index 9edace076ddb..060504719904 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h

@@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
  * known size.
  */
 #define __strscpy0(dst, src, ...)      \
-       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
+       sized_strscpy(dst, src, ARRAY_SIZE(dst))
 #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)

 #define __strscpy_pad0(dst, src, ...)  \
-       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
+       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
 #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)

 /**

--
Regards

Yafang

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Kees Cook]

On August 28, 2024 6:40:35 AM PDT, Yafang Shao <laoar.shao@gmail.com> wrote:
>On Wed, Aug 28, 2024 at 8:58 PM Alejandro Colomar <alx@kernel.org> wrote:
>>
>> On Wed, Aug 28, 2024 at 12:15:40PM GMT, Alejandro Colomar wrote:
>> > Hi Yafang,
>> >
>> > On Wed, Aug 28, 2024 at 11:03:14AM GMT, Yafang Shao wrote:
>> > > We want to eliminate the use of __get_task_comm() for the following
>> > > reasons:
>> > >
>> > > - The task_lock() is unnecessary
>> > >   Quoted from Linus [0]:
>> > >   : Since user space can randomly change their names anyway, using locking
>> > >   : was always wrong for readers (for writers it probably does make sense
>> > >   : to have some lock - although practically speaking nobody cares there
>> > >   : either, but at least for a writer some kind of race could have
>> > >   : long-term mixed results
>> > >
>> > > - The BUILD_BUG_ON() doesn't add any value
>> > >   The only requirement is to ensure that the destination buffer is a valid
>> > >   array.
>> > >
>> > > - Zeroing is not necessary in current use cases
>> > >   To avoid confusion, we should remove it. Moreover, not zeroing could
>> > >   potentially make it easier to uncover bugs. If the caller needs a
>> > >   zero-padded task name, it should be explicitly handled at the call site.
>> > >
>> > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> > > Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
>> > > Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
>> > > Suggested-by: Alejandro Colomar <alx@kernel.org>
>> > > Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
>> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>> > > Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>> > > Cc: Christian Brauner <brauner@kernel.org>
>> > > Cc: Jan Kara <jack@suse.cz>
>> > > Cc: Eric Biederman <ebiederm@xmission.com>
>> > > Cc: Kees Cook <keescook@chromium.org>
>> > > Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>> > > Cc: Matus Jokay <matus.jokay@stuba.sk>
>> > > Cc: Alejandro Colomar <alx@kernel.org>
>> > > Cc: "Serge E. Hallyn" <serge@hallyn.com>
>> > > ---
>> > >  fs/exec.c             | 10 ----------
>> > >  fs/proc/array.c       |  2 +-
>> > >  include/linux/sched.h | 32 ++++++++++++++++++++++++++------
>> > >  kernel/kthread.c      |  2 +-
>> > >  4 files changed, 28 insertions(+), 18 deletions(-)
>> > >
>> >
>> > [...]
>> >
>> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
>> > > index f8d150343d42..c40b95a79d80 100644
>> > > --- a/include/linux/sched.h
>> > > +++ b/include/linux/sched.h
>> >
>> > [...]
>> >
>> > > @@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
>> > >     __set_task_comm(tsk, from, false);
>> > >  }
>> > >
>> > > -extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
>> > > +/*
>> >
>> > [...]
>> >
>> > > + * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
>> > > + */
>> > >  #define get_task_comm(buf, tsk) ({                 \
>> > > -   BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
>> > > -   __get_task_comm(buf, sizeof(buf), tsk);         \
>> > > +   strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));     \
>> >
>> > I see that there's a two-argument macro
>> >
>> >       #define strscpy(dst, src)       sized_strscpy(dst, src, sizeof(dst))
>> >
>> > which is used in patch 2/8
>> >
>> >       diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >       index 6f0d6fb6523f..e4ef5e57dde9 100644
>> >       --- a/kernel/auditsc.c
>> >       +++ b/kernel/auditsc.c
>> >       @@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
>> >               context->target_uid = task_uid(t);
>> >               context->target_sessionid = audit_get_sessionid(t);
>> >               security_task_getsecid_obj(t, &context->target_sid);
>> >       -       memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
>> >       +       strscpy(context->target_comm, t->comm);
>> >        }
>> >
>> >        /**
>>
>> Ahh, the actual generic definition is in <include/linux/string.h>.
>> You could do
>>
>>         diff --git i/include/linux/string.h w/include/linux/string.h
>>         index 9edace076ddb..060504719904 100644
>>         --- i/include/linux/string.h
>>         +++ w/include/linux/string.h
>>         @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>>           * known size.
>>           */
>>          #define __strscpy0(dst, src, ...)      \
>>         -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
>>         +       sized_strscpy(dst, src, ARRAY_SIZE(dst))
>>          #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
>>
>>          #define __strscpy_pad0(dst, src, ...)  \
>>         -       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
>>         +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
>>          #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
>>
>>          /**
>
>Thank you for your suggestion. How does the following commit log look
>to you? Does it meet your expectations?
>
>    string: Use ARRAY_SIZE() in strscpy()
>
>    We can use ARRAY_SIZE() instead to clarify that they are regular characters.
>
>    Co-developed-by: Alejandro Colomar <alx@kernel.org>
>    Signed-off-by: Alejandro Colomar <alx@kernel.org>
>    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>
>diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
>index bbab79c0c074..07216996e3a9 100644
>--- a/arch/um/include/shared/user.h
>+++ b/arch/um/include/shared/user.h
>@@ -14,7 +14,7 @@
>  * copying too much infrastructure for my taste, so userspace files
>  * get less checking than kernel files.
>  */
>-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
>
> /* This is to get size_t and NULL */
> #ifndef __UM_HOST__
>@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
>const char *prefix_str,
> extern int in_aton(char *str);
> extern size_t strlcat(char *, const char *, size_t);
> extern size_t sized_strscpy(char *, const char *, size_t);
>-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
>+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))

Uh, but why? strscpy() copies bytes, not array elements. Using sizeof() is already correct and using ARRAY_SIZE() could lead to unexpectedly small counts (in admittedly odd situations).

What is the problem you're trying to solve here?

-Kees

-- 
Kees Cook

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

Hi Kees,

On Wed, Aug 28, 2024 at 06:48:39AM GMT, Kees Cook wrote:

[...]

> >Thank you for your suggestion. How does the following commit log look
> >to you? Does it meet your expectations?
> >
> >    string: Use ARRAY_SIZE() in strscpy()
> >
> >    We can use ARRAY_SIZE() instead to clarify that they are regular characters.
> >
> >    Co-developed-by: Alejandro Colomar <alx@kernel.org>
> >    Signed-off-by: Alejandro Colomar <alx@kernel.org>
> >    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >
> >diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
> >index bbab79c0c074..07216996e3a9 100644
> >--- a/arch/um/include/shared/user.h
> >+++ b/arch/um/include/shared/user.h
> >@@ -14,7 +14,7 @@
> >  * copying too much infrastructure for my taste, so userspace files
> >  * get less checking than kernel files.
> >  */
> >-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
> >
> > /* This is to get size_t and NULL */
> > #ifndef __UM_HOST__
> >@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
> >const char *prefix_str,
> > extern int in_aton(char *str);
> > extern size_t strlcat(char *, const char *, size_t);
> > extern size_t sized_strscpy(char *, const char *, size_t);
> >-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
> >+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))
> 
> Uh, but why? strscpy() copies bytes, not array elements. Using sizeof() is already correct and using ARRAY_SIZE() could lead to unexpectedly small counts (in admittedly odd situations).
> 
> What is the problem you're trying to solve here?

I suggested that here:
<https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq/>

There, you'll find the rationale (and also for avoiding the _pad calls
where not necessary --I ignore if it's necessary here--).

Have a lovely day!
Alex

> 
> -Kees
> 
> -- 
> Kees Cook

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Kees Cook]

On Wed, Aug 28, 2024 at 05:09:08PM +0200, Alejandro Colomar wrote:
> Hi Kees,
> 
> On Wed, Aug 28, 2024 at 06:48:39AM GMT, Kees Cook wrote:
> 
> [...]
> 
> > >Thank you for your suggestion. How does the following commit log look
> > >to you? Does it meet your expectations?
> > >
> > >    string: Use ARRAY_SIZE() in strscpy()
> > >
> > >    We can use ARRAY_SIZE() instead to clarify that they are regular characters.
> > >
> > >    Co-developed-by: Alejandro Colomar <alx@kernel.org>
> > >    Signed-off-by: Alejandro Colomar <alx@kernel.org>
> > >    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > >
> > >diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
> > >index bbab79c0c074..07216996e3a9 100644
> > >--- a/arch/um/include/shared/user.h
> > >+++ b/arch/um/include/shared/user.h
> > >@@ -14,7 +14,7 @@
> > >  * copying too much infrastructure for my taste, so userspace files
> > >  * get less checking than kernel files.
> > >  */
> > >-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > >+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
> > >
> > > /* This is to get size_t and NULL */
> > > #ifndef __UM_HOST__
> > >@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
> > >const char *prefix_str,
> > > extern int in_aton(char *str);
> > > extern size_t strlcat(char *, const char *, size_t);
> > > extern size_t sized_strscpy(char *, const char *, size_t);
> > >-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
> > >+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))
> > 
> > Uh, but why? strscpy() copies bytes, not array elements. Using sizeof() is already correct and using ARRAY_SIZE() could lead to unexpectedly small counts (in admittedly odd situations).
> > 
> > What is the problem you're trying to solve here?
> 
> I suggested that here:
> <https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq/>
> 
> There, you'll find the rationale (and also for avoiding the _pad calls
> where not necessary --I ignore if it's necessary here--).

Right, so we only use byte strings for strscpy(), so sizeof() is
sufficient. There's no technical need to switch to ARRAY_SIZE(), and I'd
like to minimize any changes to such core APIs without a good reason.

And for the _pad change, we are also doing strncpy() replacement via
case-by-case analysis, but with a common function like get_task_comm(),
I don't want to change the behavior without a complete audit of the
padding needs of every caller. Since that's rather a lot for this series,
I'd rather we just leave the existing behavior as-is, and if padding
removal is wanted after that, we can do it on a case-by-case basis then.

-Kees

-- 
Kees Cook

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 3483 bytes --]

Hi Kees,

On Wed, Aug 28, 2024 at 05:17:55PM GMT, Kees Cook wrote:
> On Wed, Aug 28, 2024 at 05:09:08PM +0200, Alejandro Colomar wrote:
> > Hi Kees,
> > 
> > On Wed, Aug 28, 2024 at 06:48:39AM GMT, Kees Cook wrote:
> > 
> > [...]
> > 
> > > >Thank you for your suggestion. How does the following commit log look
> > > >to you? Does it meet your expectations?
> > > >
> > > >    string: Use ARRAY_SIZE() in strscpy()
> > > >
> > > >    We can use ARRAY_SIZE() instead to clarify that they are regular characters.
> > > >
> > > >    Co-developed-by: Alejandro Colomar <alx@kernel.org>
> > > >    Signed-off-by: Alejandro Colomar <alx@kernel.org>
> > > >    Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > >
> > > >diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
> > > >index bbab79c0c074..07216996e3a9 100644
> > > >--- a/arch/um/include/shared/user.h
> > > >+++ b/arch/um/include/shared/user.h
> > > >@@ -14,7 +14,7 @@
> > > >  * copying too much infrastructure for my taste, so userspace files
> > > >  * get less checking than kernel files.
> > > >  */
> > > >-#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> > > >+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
> > > >
> > > > /* This is to get size_t and NULL */
> > > > #ifndef __UM_HOST__
> > > >@@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
> > > >const char *prefix_str,
> > > > extern int in_aton(char *str);
> > > > extern size_t strlcat(char *, const char *, size_t);
> > > > extern size_t sized_strscpy(char *, const char *, size_t);
> > > >-#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
> > > >+#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))
> > > 
> > > Uh, but why? strscpy() copies bytes, not array elements. Using sizeof() is already correct and using ARRAY_SIZE() could lead to unexpectedly small counts (in admittedly odd situations).
> > > 
> > > What is the problem you're trying to solve here?
> > 
> > I suggested that here:
> > <https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq/>
> > 
> > There, you'll find the rationale (and also for avoiding the _pad calls
> > where not necessary --I ignore if it's necessary here--).
> 
> Right, so we only use byte strings for strscpy(), so sizeof() is
> sufficient. There's no technical need to switch to ARRAY_SIZE(), and I'd
> like to minimize any changes to such core APIs without a good reason.

Makes sense.  My original proposal was ignoring that the wrapper was
already using __must_be_array().  Having already sizeof() +
__must_be_array(), I'd leave it like that, since both do effectively the
same.

> And for the _pad change, we are also doing strncpy() replacement via
> case-by-case analysis, but with a common function like get_task_comm(),
> I don't want to change the behavior without a complete audit of the
> padding needs of every caller.

Agree.  I had the same problem with shadow.  Removing padding was the
worst part, because it was hard to justify that nothing was relying on
the padding.

> Since that's rather a lot for this series,
> I'd rather we just leave the existing behavior as-is, and if padding
> removal is wanted after that, we can do it on a case-by-case basis then.
> 
> -Kees

Have a lovely night!
Alex

> 
> -- 
> Kees Cook

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 4279 bytes --]

Hi Yafang,

On Wed, Aug 28, 2024 at 09:40:35PM GMT, Yafang Shao wrote:
> > Ahh, the actual generic definition is in <include/linux/string.h>.
> > You could do
> >
> >         diff --git i/include/linux/string.h w/include/linux/string.h
> >         index 9edace076ddb..060504719904 100644
> >         --- i/include/linux/string.h
> >         +++ w/include/linux/string.h
> >         @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
> >           * known size.
> >           */
> >          #define __strscpy0(dst, src, ...)      \
> >         -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> >         +       sized_strscpy(dst, src, ARRAY_SIZE(dst))
> >          #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
> >
> >          #define __strscpy_pad0(dst, src, ...)  \
> >         -       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
> >         +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
> >          #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)
> >
> >          /**
> 
> Thank you for your suggestion. How does the following commit log look
> to you? Does it meet your expectations?
> 
>     string: Use ARRAY_SIZE() in strscpy()
> 
>     We can use ARRAY_SIZE() instead to clarify that they are regular characters.

I would write the following:

For symmetry with wide-character string functions, ARRAY_SIZE() is more
appropriate than sizeof().

For example, one would call wcs*cpy(dst, src, ARRAY_SIZE(dst)).
The call wcs*cpy(dst, src, sizeof(dst)) would be bogus, since the
argument is the number of wide characters, not the number of bytes.

When translating that to normal characters, one wants conceptually the
same operation, but on (normal) characters.  That is, one wants
strscpy(dst, src, ARRAY_SIZE(dst)).  While strscpy() with sizeof() works
just fine because sizeof(char)==1 by definition, it is conceptually
wrong to use it.

By using ARRAY_SIZE(), we get the __must_be_array() check for free.

> 
>     Co-developed-by: Alejandro Colomar <alx@kernel.org>
>     Signed-off-by: Alejandro Colomar <alx@kernel.org>
>     Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> 
> diff --git a/arch/um/include/shared/user.h b/arch/um/include/shared/user.h
> index bbab79c0c074..07216996e3a9 100644
> --- a/arch/um/include/shared/user.h
> +++ b/arch/um/include/shared/user.h
> @@ -14,7 +14,7 @@
>   * copying too much infrastructure for my taste, so userspace files
>   * get less checking than kernel files.
>   */
> -#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + __must_be_array(x))
> 
>  /* This is to get size_t and NULL */
>  #ifndef __UM_HOST__
> @@ -60,7 +60,7 @@ static inline void print_hex_dump(const char *level,
> const char *prefix_str,
>  extern int in_aton(char *str);
>  extern size_t strlcat(char *, const char *, size_t);
>  extern size_t sized_strscpy(char *, const char *, size_t);
> -#define strscpy(dst, src)      sized_strscpy(dst, src, sizeof(dst))
> +#define strscpy(dst, src)      sized_strscpy(dst, src, ARRAY_SIZE(dst))
> 
>  /* Copied from linux/compiler-gcc.h since we can't include it directly */
>  #define barrier() __asm__ __volatile__("": : :"memory")
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 9edace076ddb..060504719904 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> 
> @@ -76,11 +76,11 @@ ssize_t sized_strscpy(char *, const char *, size_t);
>   * known size.
>   */
>  #define __strscpy0(dst, src, ...)      \
> -       sized_strscpy(dst, src, sizeof(dst) + __must_be_array(dst))
> +       sized_strscpy(dst, src, ARRAY_SIZE(dst))
>  #define __strscpy1(dst, src, size)     sized_strscpy(dst, src, size)
> 
>  #define __strscpy_pad0(dst, src, ...)  \
> -       sized_strscpy_pad(dst, src, sizeof(dst) + __must_be_array(dst))
> +       sized_strscpy_pad(dst, src, ARRAY_SIZE(dst))
>  #define __strscpy_pad1(dst, src, size) sized_strscpy_pad(dst, src, size)

The diff looks good to me.  Thanks!

Cheers,
Alex

> 
>  /**
> 
> --
> Regards
> 
> Yafang

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Kees Cook]

On August 27, 2024 8:03:14 PM PDT, Yafang Shao <laoar.shao@gmail.com> wrote:
>We want to eliminate the use of __get_task_comm() for the following
>reasons:
>
>- The task_lock() is unnecessary
>  Quoted from Linus [0]:
>  : Since user space can randomly change their names anyway, using locking
>  : was always wrong for readers (for writers it probably does make sense
>  : to have some lock - although practically speaking nobody cares there
>  : either, but at least for a writer some kind of race could have
>  : long-term mixed results
>
>- The BUILD_BUG_ON() doesn't add any value
>  The only requirement is to ensure that the destination buffer is a valid
>  array.

Sorry, that's not a correct evaluation. See below.

>
>- Zeroing is not necessary in current use cases
>  To avoid confusion, we should remove it. Moreover, not zeroing could
>  potentially make it easier to uncover bugs. If the caller needs a
>  zero-padded task name, it should be explicitly handled at the call site.

This is also not an appropriate rationale. We don't make the kernel "more buggy" not purpose. ;) See below.

>
>Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
>Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
>Suggested-by: Alejandro Colomar <alx@kernel.org>
>Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
>Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
>Cc: Alexander Viro <viro@zeniv.linux.org.uk>
>Cc: Christian Brauner <brauner@kernel.org>
>Cc: Jan Kara <jack@suse.cz>
>Cc: Eric Biederman <ebiederm@xmission.com>
>Cc: Kees Cook <keescook@chromium.org>
>Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>Cc: Matus Jokay <matus.jokay@stuba.sk>
>Cc: Alejandro Colomar <alx@kernel.org>
>Cc: "Serge E. Hallyn" <serge@hallyn.com>
>---
> fs/exec.c             | 10 ----------
> fs/proc/array.c       |  2 +-
> include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> kernel/kthread.c      |  2 +-
> 4 files changed, 28 insertions(+), 18 deletions(-)
>
>diff --git a/fs/exec.c b/fs/exec.c
>index 50e76cc633c4..8a23171bc3c3 100644
>--- a/fs/exec.c
>+++ b/fs/exec.c
>@@ -1264,16 +1264,6 @@ static int unshare_sighand(struct task_struct *me)
> 	return 0;
> }
> 
>-char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
>-{
>-	task_lock(tsk);
>-	/* Always NUL terminated and zero-padded */
>-	strscpy_pad(buf, tsk->comm, buf_size);
>-	task_unlock(tsk);
>-	return buf;
>-}
>-EXPORT_SYMBOL_GPL(__get_task_comm);
>-
> /*
>  * These functions flushes out all traces of the currently running executable
>  * so that a new one can be started
>diff --git a/fs/proc/array.c b/fs/proc/array.c
>index 34a47fb0c57f..55ed3510d2bb 100644
>--- a/fs/proc/array.c
>+++ b/fs/proc/array.c
>@@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
> 	else if (p->flags & PF_KTHREAD)
> 		get_kthread_comm(tcomm, sizeof(tcomm), p);
> 	else
>-		__get_task_comm(tcomm, sizeof(tcomm), p);
>+		get_task_comm(tcomm, p);
> 
> 	if (escape)
> 		seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
>diff --git a/include/linux/sched.h b/include/linux/sched.h
>index f8d150343d42..c40b95a79d80 100644
>--- a/include/linux/sched.h
>+++ b/include/linux/sched.h
>@@ -1096,9 +1096,12 @@ struct task_struct {
> 	/*
> 	 * executable name, excluding path.
> 	 *
>-	 * - normally initialized setup_new_exec()
>-	 * - access it with [gs]et_task_comm()
>-	 * - lock it with task_lock()
>+	 * - normally initialized begin_new_exec()
>+	 * - set it with set_task_comm()
>+	 *   - strscpy_pad() to ensure it is always NUL-terminated and
>+	 *     zero-padded
>+	 *   - task_lock() to ensure the operation is atomic and the name is
>+	 *     fully updated.
> 	 */
> 	char				comm[TASK_COMM_LEN];
> 
>@@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> 	__set_task_comm(tsk, from, false);
> }
> 
>-extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
>+/*
>+ * - Why not use task_lock()?
>+ *   User space can randomly change their names anyway, so locking for readers
>+ *   doesn't make sense. For writers, locking is probably necessary, as a race
>+ *   condition could lead to long-term mixed results.
>+ *   The strscpy_pad() in __set_task_comm() can ensure that the task comm is
>+ *   always NUL-terminated and zero-padded. Therefore the race condition between
>+ *   reader and writer is not an issue.
>+ *
>+ * - Why not use strscpy_pad()?
>+ *   While strscpy_pad() prevents writing garbage past the NUL terminator, which
>+ *   is useful when using the task name as a key in a hash map, most use cases
>+ *   don't require this. Zero-padding might confuse users if it’s unnecessary,
>+ *   and not zeroing might even make it easier to expose bugs. If you need a
>+ *   zero-padded task name, please handle that explicitly at the call site.

I really don't like this part of the change. You don't know that existing callers don't depend on the padding. Please invert this logic: get_task_comm() must use strscpy_pad(). Calls NOT wanting padding can call strscpy() themselves.

>+ *
>+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array.

This doesn't need checking here; strscpy() will already do that. 

>+ */
> #define get_task_comm(buf, tsk) ({			\
>-	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\

Also, please leave the TASK_COMM_LEN test so that destination buffers continue to be the correct size: current callers do not perform any return value analysis, so they cannot accidentally start having situations where the destination string might be truncated. Again, anyone wanting to avoid that restriction can use strscpy() directly and check the return value.

>-	__get_task_comm(buf, sizeof(buf), tsk);		\
>+	strscpy(buf, (tsk)->comm, ARRAY_SIZE(buf));	\
>+	buf;						\
> })
> 
> #ifdef CONFIG_SMP
>diff --git a/kernel/kthread.c b/kernel/kthread.c
>index f7be976ff88a..7d001d033cf9 100644
>--- a/kernel/kthread.c
>+++ b/kernel/kthread.c
>@@ -101,7 +101,7 @@ void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> 	struct kthread *kthread = to_kthread(tsk);
> 
> 	if (!kthread || !kthread->full_name) {
>-		__get_task_comm(buf, buf_size, tsk);
>+		strscpy(buf, tsk->comm, buf_size);

Why is this safe to not use strscpy_pad? Also, is buf_size ever not TASK_COMM_LEN?

> 		return;
> 	}
> 

-- 
Kees Cook

Re: [PATCH v8 1/8] Get rid of __get_task_comm()

[Yafang Shao]

On Wed, Aug 28, 2024 at 10:04 PM Kees Cook <kees@kernel.org> wrote:
>
>
>
> On August 27, 2024 8:03:14 PM PDT, Yafang Shao <laoar.shao@gmail.com> wrote:
> >We want to eliminate the use of __get_task_comm() for the following
> >reasons:
> >
> >- The task_lock() is unnecessary
> >  Quoted from Linus [0]:
> >  : Since user space can randomly change their names anyway, using locking
> >  : was always wrong for readers (for writers it probably does make sense
> >  : to have some lock - although practically speaking nobody cares there
> >  : either, but at least for a writer some kind of race could have
> >  : long-term mixed results
> >
> >- The BUILD_BUG_ON() doesn't add any value
> >  The only requirement is to ensure that the destination buffer is a valid
> >  array.
>
> Sorry, that's not a correct evaluation. See below.
>
> >
> >- Zeroing is not necessary in current use cases
> >  To avoid confusion, we should remove it. Moreover, not zeroing could
> >  potentially make it easier to uncover bugs. If the caller needs a
> >  zero-padded task name, it should be explicitly handled at the call site.
>
> This is also not an appropriate rationale. We don't make the kernel "more buggy" not purpose. ;) See below.
>
> >
> >Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> >Link: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com [0]
> >Link: https://lore.kernel.org/all/CAHk-=whWtUC-AjmGJveAETKOMeMFSTwKwu99v7+b6AyHMmaDFA@mail.gmail.com/
> >Suggested-by: Alejandro Colomar <alx@kernel.org>
> >Link: https://lore.kernel.org/all/2jxak5v6dfxlpbxhpm3ey7oup4g2lnr3ueurfbosf5wdo65dk4@srb3hsk72zwq
> >Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> >Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> >Cc: Christian Brauner <brauner@kernel.org>
> >Cc: Jan Kara <jack@suse.cz>
> >Cc: Eric Biederman <ebiederm@xmission.com>
> >Cc: Kees Cook <keescook@chromium.org>
> >Cc: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> >Cc: Matus Jokay <matus.jokay@stuba.sk>
> >Cc: Alejandro Colomar <alx@kernel.org>
> >Cc: "Serge E. Hallyn" <serge@hallyn.com>
> >---
> > fs/exec.c             | 10 ----------
> > fs/proc/array.c       |  2 +-
> > include/linux/sched.h | 32 ++++++++++++++++++++++++++------
> > kernel/kthread.c      |  2 +-
> > 4 files changed, 28 insertions(+), 18 deletions(-)
> >
> >diff --git a/fs/exec.c b/fs/exec.c
> >index 50e76cc633c4..8a23171bc3c3 100644
> >--- a/fs/exec.c
> >+++ b/fs/exec.c
> >@@ -1264,16 +1264,6 @@ static int unshare_sighand(struct task_struct *me)
> >       return 0;
> > }
> >
> >-char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> >-{
> >-      task_lock(tsk);
> >-      /* Always NUL terminated and zero-padded */
> >-      strscpy_pad(buf, tsk->comm, buf_size);
> >-      task_unlock(tsk);
> >-      return buf;
> >-}
> >-EXPORT_SYMBOL_GPL(__get_task_comm);
> >-
> > /*
> >  * These functions flushes out all traces of the currently running executable
> >  * so that a new one can be started
> >diff --git a/fs/proc/array.c b/fs/proc/array.c
> >index 34a47fb0c57f..55ed3510d2bb 100644
> >--- a/fs/proc/array.c
> >+++ b/fs/proc/array.c
> >@@ -109,7 +109,7 @@ void proc_task_name(struct seq_file *m, struct task_struct *p, bool escape)
> >       else if (p->flags & PF_KTHREAD)
> >               get_kthread_comm(tcomm, sizeof(tcomm), p);
> >       else
> >-              __get_task_comm(tcomm, sizeof(tcomm), p);
> >+              get_task_comm(tcomm, p);
> >
> >       if (escape)
> >               seq_escape_str(m, tcomm, ESCAPE_SPACE | ESCAPE_SPECIAL, "\n\\");
> >diff --git a/include/linux/sched.h b/include/linux/sched.h
> >index f8d150343d42..c40b95a79d80 100644
> >--- a/include/linux/sched.h
> >+++ b/include/linux/sched.h
> >@@ -1096,9 +1096,12 @@ struct task_struct {
> >       /*
> >        * executable name, excluding path.
> >        *
> >-       * - normally initialized setup_new_exec()
> >-       * - access it with [gs]et_task_comm()
> >-       * - lock it with task_lock()
> >+       * - normally initialized begin_new_exec()
> >+       * - set it with set_task_comm()
> >+       *   - strscpy_pad() to ensure it is always NUL-terminated and
> >+       *     zero-padded
> >+       *   - task_lock() to ensure the operation is atomic and the name is
> >+       *     fully updated.
> >        */
> >       char                            comm[TASK_COMM_LEN];
> >
> >@@ -1914,10 +1917,27 @@ static inline void set_task_comm(struct task_struct *tsk, const char *from)
> >       __set_task_comm(tsk, from, false);
> > }
> >
> >-extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> >+/*
> >+ * - Why not use task_lock()?
> >+ *   User space can randomly change their names anyway, so locking for readers
> >+ *   doesn't make sense. For writers, locking is probably necessary, as a race
> >+ *   condition could lead to long-term mixed results.
> >+ *   The strscpy_pad() in __set_task_comm() can ensure that the task comm is
> >+ *   always NUL-terminated and zero-padded. Therefore the race condition between
> >+ *   reader and writer is not an issue.
> >+ *
> >+ * - Why not use strscpy_pad()?
> >+ *   While strscpy_pad() prevents writing garbage past the NUL terminator, which
> >+ *   is useful when using the task name as a key in a hash map, most use cases
> >+ *   don't require this. Zero-padding might confuse users if it’s unnecessary,
> >+ *   and not zeroing might even make it easier to expose bugs. If you need a
> >+ *   zero-padded task name, please handle that explicitly at the call site.
>
> I really don't like this part of the change. You don't know that existing callers don't depend on the padding. Please invert this logic: get_task_comm() must use strscpy_pad(). Calls NOT wanting padding can call strscpy() themselves.
>
> >+ *
> >+ * - ARRAY_SIZE() can help ensure that @buf is indeed an array.
>
> This doesn't need checking here; strscpy() will already do that.
>
> >+ */
> > #define get_task_comm(buf, tsk) ({                    \
> >-      BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
>
> Also, please leave the TASK_COMM_LEN test so that destination buffers continue to be the correct size: current callers do not perform any return value analysis, so they cannot accidentally start having situations where the destination string might be truncated. Again, anyone wanting to avoid that restriction can use strscpy() directly and check the return value.

Hello Kees,

Thanks for your input.

Alejandro has addressed all the other changes except for the removal
of BUILD_BUG_ON(). I have a question regarding this: if we're using it
to avoid truncation, why not write it like this?

    BUILD_BUG_ON(sizeof(buf) < TASK_COMM_LEN);

This way, it ensures that the size is at least as large as TASK_COMM_LEN.

--
Regards
Yafang

[PATCH v8 2/8] auditsc: Replace memcpy() with strscpy()

[Yafang Shao]

Using strscpy() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Paul Moore <paul@paul-moore.com>
Cc: Eric Paris <eparis@redhat.com>
---
 kernel/auditsc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6f0d6fb6523f..e4ef5e57dde9 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
 	context->target_uid = task_uid(t);
 	context->target_sessionid = audit_get_sessionid(t);
 	security_task_getsecid_obj(t, &context->target_sid);
-	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
+	strscpy(context->target_comm, t->comm);
 }
 
 /**
@@ -2757,7 +2757,7 @@ int audit_signal_info_syscall(struct task_struct *t)
 		ctx->target_uid = t_uid;
 		ctx->target_sessionid = audit_get_sessionid(t);
 		security_task_getsecid_obj(t, &ctx->target_sid);
-		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
+		strscpy(ctx->target_comm, t->comm);
 		return 0;
 	}
 
@@ -2778,7 +2778,7 @@ int audit_signal_info_syscall(struct task_struct *t)
 	axp->target_uid[axp->pid_count] = t_uid;
 	axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
 	security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]);
-	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
+	strscpy(axp->target_comm[axp->pid_count], t->comm);
 	axp->pid_count++;
 
 	return 0;
-- 
2.43.5

Re: [PATCH v8 2/8] auditsc: Replace memcpy() with strscpy()

[Justin Stitt]

Hi,

On Wed, Aug 28, 2024 at 11:03:15AM GMT, Yafang Shao wrote:
> Using strscpy() to read the task comm ensures that the name is
> always NUL-terminated, regardless of the source string. This approach also
> facilitates future extensions to the task comm.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Acked-by: Paul Moore <paul@paul-moore.com>
> Cc: Eric Paris <eparis@redhat.com>
> ---
>  kernel/auditsc.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6f0d6fb6523f..e4ef5e57dde9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2730,7 +2730,7 @@ void __audit_ptrace(struct task_struct *t)
>  	context->target_uid = task_uid(t);
>  	context->target_sessionid = audit_get_sessionid(t);
>  	security_task_getsecid_obj(t, &context->target_sid);
> -	memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> +	strscpy(context->target_comm, t->comm);
>  }
>  
>  /**
> @@ -2757,7 +2757,7 @@ int audit_signal_info_syscall(struct task_struct *t)
>  		ctx->target_uid = t_uid;
>  		ctx->target_sessionid = audit_get_sessionid(t);
>  		security_task_getsecid_obj(t, &ctx->target_sid);
> -		memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> +		strscpy(ctx->target_comm, t->comm);
>  		return 0;
>  	}
>  
> @@ -2778,7 +2778,7 @@ int audit_signal_info_syscall(struct task_struct *t)
>  	axp->target_uid[axp->pid_count] = t_uid;
>  	axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
>  	security_task_getsecid_obj(t, &axp->target_sid[axp->pid_count]);
> -	memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> +	strscpy(axp->target_comm[axp->pid_count], t->comm);
>  	axp->pid_count++;
>  
>  	return 0;
> -- 
> 2.43.5
> 

Good usage of two-argument strscpy(). This helps towards [1].

Reviewed-by: Justin Stitt <justinstitt@google.com>

[1]: https://github.com/KSPP/linux/issues/90

Thanks
Justin

[PATCH v8 3/8] security: Replace memcpy() with get_task_comm()

[Yafang Shao]

Quoted from Linus [0]:

  selinux never wanted a lock, and never wanted any kind of *consistent*
  result, it just wanted a *stable* result.

Using get_task_comm() to read the task comm ensures that the name is
always NUL-terminated, regardless of the source string. This approach also
facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
LINK: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com/ [0]
Acked-by: Paul Moore <paul@paul-moore.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/lsm_audit.c         | 4 ++--
 security/selinux/selinuxfs.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 849e832719e2..9a8352972086 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -207,7 +207,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
 
 	audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
-	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
+	audit_log_untrustedstring(ab, get_task_comm(comm, current));
 
 	switch (a->type) {
 	case LSM_AUDIT_DATA_NONE:
@@ -302,7 +302,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 				char comm[sizeof(tsk->comm)];
 				audit_log_format(ab, " opid=%d ocomm=", pid);
 				audit_log_untrustedstring(ab,
-				    memcpy(comm, tsk->comm, sizeof(comm)));
+				    get_task_comm(comm, tsk));
 			}
 		}
 		break;
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index e172f182b65c..c9b05be27ddb 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -708,7 +708,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf,
 	if (new_value) {
 		char comm[sizeof(current->comm)];
 
-		memcpy(comm, current->comm, sizeof(comm));
+		strscpy(comm, current->comm);
 		pr_err("SELinux: %s (%d) set checkreqprot to 1. This is no longer supported.\n",
 		       comm, current->pid);
 	}
-- 
2.43.5

[PATCH v8 4/8] bpftool: Ensure task comm is always NUL-terminated

[Yafang Shao]

Let's explicitly ensure the destination string is NUL-terminated. This way,
it won't be affected by changes to the source string.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Reviewed-by: Quentin Monnet <qmo@kernel.org>
---
 tools/bpf/bpftool/pids.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
index 9b898571b49e..23f488cf1740 100644
--- a/tools/bpf/bpftool/pids.c
+++ b/tools/bpf/bpftool/pids.c
@@ -54,6 +54,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
 		ref = &refs->refs[refs->ref_cnt];
 		ref->pid = e->pid;
 		memcpy(ref->comm, e->comm, sizeof(ref->comm));
+		ref->comm[sizeof(ref->comm) - 1] = '\0';
 		refs->ref_cnt++;
 
 		return;
@@ -77,6 +78,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
 	ref = &refs->refs[0];
 	ref->pid = e->pid;
 	memcpy(ref->comm, e->comm, sizeof(ref->comm));
+	ref->comm[sizeof(ref->comm) - 1] = '\0';
 	refs->ref_cnt = 1;
 	refs->has_bpf_cookie = e->has_bpf_cookie;
 	refs->bpf_cookie = e->bpf_cookie;
-- 
2.43.5

Re: [PATCH v8 4/8] bpftool: Ensure task comm is always NUL-terminated

[Justin Stitt]

Hi,

On Wed, Aug 28, 2024 at 11:03:17AM GMT, Yafang Shao wrote:
> Let's explicitly ensure the destination string is NUL-terminated. This way,
> it won't be affected by changes to the source string.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Reviewed-by: Quentin Monnet <qmo@kernel.org>
> ---
>  tools/bpf/bpftool/pids.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> index 9b898571b49e..23f488cf1740 100644
> --- a/tools/bpf/bpftool/pids.c
> +++ b/tools/bpf/bpftool/pids.c
> @@ -54,6 +54,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
>  		ref = &refs->refs[refs->ref_cnt];
>  		ref->pid = e->pid;
>  		memcpy(ref->comm, e->comm, sizeof(ref->comm));
> +		ref->comm[sizeof(ref->comm) - 1] = '\0';

...

>  		refs->ref_cnt++;
>  
>  		return;
> @@ -77,6 +78,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
>  	ref = &refs->refs[0];
>  	ref->pid = e->pid;
>  	memcpy(ref->comm, e->comm, sizeof(ref->comm));
> +	ref->comm[sizeof(ref->comm) - 1] = '\0';

Excuse my ignorance, do we not have a strscpy() equivalent usable in bpf
code?

>  	refs->ref_cnt = 1;
>  	refs->has_bpf_cookie = e->has_bpf_cookie;
>  	refs->bpf_cookie = e->bpf_cookie;
> -- 
> 2.43.5
> 

Thanks
Justin

Re: [PATCH v8 4/8] bpftool: Ensure task comm is always NUL-terminated

[Yafang Shao]

On Fri, Sep 13, 2024 at 5:14 AM Justin Stitt <justinstitt@google.com> wrote:
>
> Hi,
>
> On Wed, Aug 28, 2024 at 11:03:17AM GMT, Yafang Shao wrote:
> > Let's explicitly ensure the destination string is NUL-terminated. This way,
> > it won't be affected by changes to the source string.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Reviewed-by: Quentin Monnet <qmo@kernel.org>
> > ---
> >  tools/bpf/bpftool/pids.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/pids.c b/tools/bpf/bpftool/pids.c
> > index 9b898571b49e..23f488cf1740 100644
> > --- a/tools/bpf/bpftool/pids.c
> > +++ b/tools/bpf/bpftool/pids.c
> > @@ -54,6 +54,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
> >               ref = &refs->refs[refs->ref_cnt];
> >               ref->pid = e->pid;
> >               memcpy(ref->comm, e->comm, sizeof(ref->comm));
> > +             ref->comm[sizeof(ref->comm) - 1] = '\0';
>
> ...
>
> >               refs->ref_cnt++;
> >
> >               return;
> > @@ -77,6 +78,7 @@ static void add_ref(struct hashmap *map, struct pid_iter_entry *e)
> >       ref = &refs->refs[0];
> >       ref->pid = e->pid;
> >       memcpy(ref->comm, e->comm, sizeof(ref->comm));
> > +     ref->comm[sizeof(ref->comm) - 1] = '\0';
>
> Excuse my ignorance, do we not have a strscpy() equivalent usable in bpf
> code?

To my knowledge, there is no direct equivalent of the standard
strcpy() function available in bpftool or libbpf code.

-- 
Regards
Yafang

[PATCH v8 5/8] mm/util: Fix possible race condition in kstrdup()

[Yafang Shao]

In kstrdup(), it is critical to ensure that the dest string is always
NUL-terminated. However, potential race condidtion can occur between a
writer and a reader.

Consider the following scenario involving task->comm:

    reader                    writer

  len = strlen(s) + 1;
                             strlcpy(tsk->comm, buf, sizeof(tsk->comm));
  memcpy(buf, s, len);

In this case, there is a race condition between the reader and the
writer. The reader calculate the length of the string `s` based on the
old value of task->comm. However, during the memcpy(), the string `s`
might be updated by the writer to a new value of task->comm.

If the new task->comm is larger than the old one, the `buf` might not be
NUL-terminated. This can lead to undefined behavior and potential
security vulnerabilities.

Let's fix it by explicitly adding a NUL terminator.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Alejandro Colomar <alx@kernel.org>
---
 mm/util.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/mm/util.c b/mm/util.c
index bd283e2132e0..9a77a347c385 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -62,8 +62,14 @@ char *kstrdup(const char *s, gfp_t gfp)
 
 	len = strlen(s) + 1;
 	buf = kmalloc_track_caller(len, gfp);
-	if (buf)
+	if (buf) {
 		memcpy(buf, s, len);
+		/* During memcpy(), the string might be updated to a new value,
+		 * which could be longer than the string when strlen() is
+		 * called. Therefore, we need to add a NUL termimator.
+		 */
+		buf[len - 1] = '\0';
+	}
 	return buf;
 }
 EXPORT_SYMBOL(kstrdup);
-- 
2.43.5

[PATCH v8 6/8] mm/util: Deduplicate code in {kstrdup,kstrndup,kmemdup_nul}

[Yafang Shao]

These three functions follow the same pattern. To deduplicate the code,
let's introduce a common helper __kmemdup_nul().

Suggested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Simon Horman <horms@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Alejandro Colomar <alx@kernel.org>
---
 mm/util.c | 68 ++++++++++++++++++++++---------------------------------
 1 file changed, 27 insertions(+), 41 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index 9a77a347c385..42714fe13e24 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -45,33 +45,41 @@ void kfree_const(const void *x)
 EXPORT_SYMBOL(kfree_const);
 
 /**
- * kstrdup - allocate space for and copy an existing string
- * @s: the string to duplicate
+ * __kmemdup_nul - Create a NUL-terminated string from @s, which might be unterminated.
+ * @s: The data to copy
+ * @len: The size of the data, not including the NUL terminator
  * @gfp: the GFP mask used in the kmalloc() call when allocating memory
  *
- * Return: newly allocated copy of @s or %NULL in case of error
+ * Return: newly allocated copy of @s with NUL-termination or %NULL in
+ * case of error
  */
-noinline
-char *kstrdup(const char *s, gfp_t gfp)
+static __always_inline char *__kmemdup_nul(const char *s, size_t len, gfp_t gfp)
 {
-	size_t len;
 	char *buf;
 
-	if (!s)
+	/* '+1' for the NUL terminator */
+	buf = kmalloc_track_caller(len + 1, gfp);
+	if (!buf)
 		return NULL;
 
-	len = strlen(s) + 1;
-	buf = kmalloc_track_caller(len, gfp);
-	if (buf) {
-		memcpy(buf, s, len);
-		/* During memcpy(), the string might be updated to a new value,
-		 * which could be longer than the string when strlen() is
-		 * called. Therefore, we need to add a NUL termimator.
-		 */
-		buf[len - 1] = '\0';
-	}
+	memcpy(buf, s, len);
+	/* Ensure the buf is always NUL-terminated, regardless of @s. */
+	buf[len] = '\0';
 	return buf;
 }
+
+/**
+ * kstrdup - allocate space for and copy an existing string
+ * @s: the string to duplicate
+ * @gfp: the GFP mask used in the kmalloc() call when allocating memory
+ *
+ * Return: newly allocated copy of @s or %NULL in case of error
+ */
+noinline
+char *kstrdup(const char *s, gfp_t gfp)
+{
+	return s ? __kmemdup_nul(s, strlen(s), gfp) : NULL;
+}
 EXPORT_SYMBOL(kstrdup);
 
 /**
@@ -106,19 +114,7 @@ EXPORT_SYMBOL(kstrdup_const);
  */
 char *kstrndup(const char *s, size_t max, gfp_t gfp)
 {
-	size_t len;
-	char *buf;
-
-	if (!s)
-		return NULL;
-
-	len = strnlen(s, max);
-	buf = kmalloc_track_caller(len+1, gfp);
-	if (buf) {
-		memcpy(buf, s, len);
-		buf[len] = '\0';
-	}
-	return buf;
+	return s ? __kmemdup_nul(s, strnlen(s, max), gfp) : NULL;
 }
 EXPORT_SYMBOL(kstrndup);
 
@@ -192,17 +188,7 @@ EXPORT_SYMBOL(kvmemdup);
  */
 char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
 {
-	char *buf;
-
-	if (!s)
-		return NULL;
-
-	buf = kmalloc_track_caller(len + 1, gfp);
-	if (buf) {
-		memcpy(buf, s, len);
-		buf[len] = '\0';
-	}
-	return buf;
+	return s ? __kmemdup_nul(s, len, gfp) : NULL;
 }
 EXPORT_SYMBOL(kmemdup_nul);
 
-- 
2.43.5

Re: [PATCH v8 6/8] mm/util: Deduplicate code in {kstrdup,kstrndup,kmemdup_nul}

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 3596 bytes --]

On Wed, Aug 28, 2024 at 11:03:19AM GMT, Yafang Shao wrote:
> These three functions follow the same pattern. To deduplicate the code,
> let's introduce a common helper __kmemdup_nul().
> 
> Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Simon Horman <horms@kernel.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Alejandro Colomar <alx@kernel.org>
> ---

Reviewed-by: Alejandro Colomar <alx@kernel.org>

Cheers,
Alex

>  mm/util.c | 68 ++++++++++++++++++++++---------------------------------
>  1 file changed, 27 insertions(+), 41 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index 9a77a347c385..42714fe13e24 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -45,33 +45,41 @@ void kfree_const(const void *x)
>  EXPORT_SYMBOL(kfree_const);
>  
>  /**
> - * kstrdup - allocate space for and copy an existing string
> - * @s: the string to duplicate
> + * __kmemdup_nul - Create a NUL-terminated string from @s, which might be unterminated.
> + * @s: The data to copy
> + * @len: The size of the data, not including the NUL terminator
>   * @gfp: the GFP mask used in the kmalloc() call when allocating memory
>   *
> - * Return: newly allocated copy of @s or %NULL in case of error
> + * Return: newly allocated copy of @s with NUL-termination or %NULL in
> + * case of error
>   */
> -noinline
> -char *kstrdup(const char *s, gfp_t gfp)
> +static __always_inline char *__kmemdup_nul(const char *s, size_t len, gfp_t gfp)
>  {
> -	size_t len;
>  	char *buf;
>  
> -	if (!s)
> +	/* '+1' for the NUL terminator */
> +	buf = kmalloc_track_caller(len + 1, gfp);
> +	if (!buf)
>  		return NULL;
>  
> -	len = strlen(s) + 1;
> -	buf = kmalloc_track_caller(len, gfp);
> -	if (buf) {
> -		memcpy(buf, s, len);
> -		/* During memcpy(), the string might be updated to a new value,
> -		 * which could be longer than the string when strlen() is
> -		 * called. Therefore, we need to add a NUL termimator.
> -		 */
> -		buf[len - 1] = '\0';
> -	}
> +	memcpy(buf, s, len);
> +	/* Ensure the buf is always NUL-terminated, regardless of @s. */
> +	buf[len] = '\0';
>  	return buf;
>  }
> +
> +/**
> + * kstrdup - allocate space for and copy an existing string
> + * @s: the string to duplicate
> + * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> + *
> + * Return: newly allocated copy of @s or %NULL in case of error
> + */
> +noinline
> +char *kstrdup(const char *s, gfp_t gfp)
> +{
> +	return s ? __kmemdup_nul(s, strlen(s), gfp) : NULL;
> +}
>  EXPORT_SYMBOL(kstrdup);
>  
>  /**
> @@ -106,19 +114,7 @@ EXPORT_SYMBOL(kstrdup_const);
>   */
>  char *kstrndup(const char *s, size_t max, gfp_t gfp)
>  {
> -	size_t len;
> -	char *buf;
> -
> -	if (!s)
> -		return NULL;
> -
> -	len = strnlen(s, max);
> -	buf = kmalloc_track_caller(len+1, gfp);
> -	if (buf) {
> -		memcpy(buf, s, len);
> -		buf[len] = '\0';
> -	}
> -	return buf;
> +	return s ? __kmemdup_nul(s, strnlen(s, max), gfp) : NULL;
>  }
>  EXPORT_SYMBOL(kstrndup);
>  
> @@ -192,17 +188,7 @@ EXPORT_SYMBOL(kvmemdup);
>   */
>  char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
>  {
> -	char *buf;
> -
> -	if (!s)
> -		return NULL;
> -
> -	buf = kmalloc_track_caller(len + 1, gfp);
> -	if (buf) {
> -		memcpy(buf, s, len);
> -		buf[len] = '\0';
> -	}
> -	return buf;
> +	return s ? __kmemdup_nul(s, len, gfp) : NULL;
>  }
>  EXPORT_SYMBOL(kmemdup_nul);
>  
> -- 
> 2.43.5
> 

-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 6/8] mm/util: Deduplicate code in {kstrdup,kstrndup,kmemdup_nul}

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]

On Wed, Aug 28, 2024 at 12:32:53PM GMT, Alejandro Colomar wrote:
> On Wed, Aug 28, 2024 at 11:03:19AM GMT, Yafang Shao wrote:
> > These three functions follow the same pattern. To deduplicate the code,
> > let's introduce a common helper __kmemdup_nul().
> > 
> > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Simon Horman <horms@kernel.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Alejandro Colomar <alx@kernel.org>
> > ---
> 
> Reviewed-by: Alejandro Colomar <alx@kernel.org>

(Or maybe I should say

Co-developed-by: Alejandro Colomar <alx@kernel.org>
Signed-off-by: Alejandro Colomar <alx@kernel.org>

)


> 
> Cheers,
> Alex
> 
> >  mm/util.c | 68 ++++++++++++++++++++++---------------------------------
> >  1 file changed, 27 insertions(+), 41 deletions(-)
> > 
> > diff --git a/mm/util.c b/mm/util.c
> > index 9a77a347c385..42714fe13e24 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -45,33 +45,41 @@ void kfree_const(const void *x)
> >  EXPORT_SYMBOL(kfree_const);
> >  
> >  /**
> > - * kstrdup - allocate space for and copy an existing string
> > - * @s: the string to duplicate
> > + * __kmemdup_nul - Create a NUL-terminated string from @s, which might be unterminated.
> > + * @s: The data to copy
> > + * @len: The size of the data, not including the NUL terminator
> >   * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> >   *
> > - * Return: newly allocated copy of @s or %NULL in case of error
> > + * Return: newly allocated copy of @s with NUL-termination or %NULL in
> > + * case of error
> >   */
> > -noinline
> > -char *kstrdup(const char *s, gfp_t gfp)
> > +static __always_inline char *__kmemdup_nul(const char *s, size_t len, gfp_t gfp)
> >  {
> > -	size_t len;
> >  	char *buf;
> >  
> > -	if (!s)
> > +	/* '+1' for the NUL terminator */
> > +	buf = kmalloc_track_caller(len + 1, gfp);
> > +	if (!buf)
> >  		return NULL;
> >  
> > -	len = strlen(s) + 1;
> > -	buf = kmalloc_track_caller(len, gfp);
> > -	if (buf) {
> > -		memcpy(buf, s, len);
> > -		/* During memcpy(), the string might be updated to a new value,
> > -		 * which could be longer than the string when strlen() is
> > -		 * called. Therefore, we need to add a NUL termimator.
> > -		 */
> > -		buf[len - 1] = '\0';
> > -	}
> > +	memcpy(buf, s, len);
> > +	/* Ensure the buf is always NUL-terminated, regardless of @s. */
> > +	buf[len] = '\0';
> >  	return buf;
> >  }
> > +
> > +/**
> > + * kstrdup - allocate space for and copy an existing string
> > + * @s: the string to duplicate
> > + * @gfp: the GFP mask used in the kmalloc() call when allocating memory
> > + *
> > + * Return: newly allocated copy of @s or %NULL in case of error
> > + */
> > +noinline
> > +char *kstrdup(const char *s, gfp_t gfp)
> > +{
> > +	return s ? __kmemdup_nul(s, strlen(s), gfp) : NULL;
> > +}
> >  EXPORT_SYMBOL(kstrdup);
> >  
> >  /**
> > @@ -106,19 +114,7 @@ EXPORT_SYMBOL(kstrdup_const);
> >   */
> >  char *kstrndup(const char *s, size_t max, gfp_t gfp)
> >  {
> > -	size_t len;
> > -	char *buf;
> > -
> > -	if (!s)
> > -		return NULL;
> > -
> > -	len = strnlen(s, max);
> > -	buf = kmalloc_track_caller(len+1, gfp);
> > -	if (buf) {
> > -		memcpy(buf, s, len);
> > -		buf[len] = '\0';
> > -	}
> > -	return buf;
> > +	return s ? __kmemdup_nul(s, strnlen(s, max), gfp) : NULL;
> >  }
> >  EXPORT_SYMBOL(kstrndup);
> >  
> > @@ -192,17 +188,7 @@ EXPORT_SYMBOL(kvmemdup);
> >   */
> >  char *kmemdup_nul(const char *s, size_t len, gfp_t gfp)
> >  {
> > -	char *buf;
> > -
> > -	if (!s)
> > -		return NULL;
> > -
> > -	buf = kmalloc_track_caller(len + 1, gfp);
> > -	if (buf) {
> > -		memcpy(buf, s, len);
> > -		buf[len] = '\0';
> > -	}
> > -	return buf;
> > +	return s ? __kmemdup_nul(s, len, gfp) : NULL;
> >  }
> >  EXPORT_SYMBOL(kmemdup_nul);
> >  
> > -- 
> > 2.43.5
> > 
> 
> -- 
> <https://www.alejandro-colomar.es/>



-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v8 6/8] mm/util: Deduplicate code in {kstrdup,kstrndup,kmemdup_nul}

[Yafang Shao]

On Wed, Aug 28, 2024 at 6:33 PM Alejandro Colomar <alx@kernel.org> wrote:
>
> On Wed, Aug 28, 2024 at 12:32:53PM GMT, Alejandro Colomar wrote:
> > On Wed, Aug 28, 2024 at 11:03:19AM GMT, Yafang Shao wrote:
> > > These three functions follow the same pattern. To deduplicate the code,
> > > let's introduce a common helper __kmemdup_nul().
> > >
> > > Suggested-by: Andrew Morton <akpm@linux-foundation.org>
> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > Cc: Simon Horman <horms@kernel.org>
> > > Cc: Matthew Wilcox <willy@infradead.org>
> > > Cc: Alejandro Colomar <alx@kernel.org>
> > > ---
> >
> > Reviewed-by: Alejandro Colomar <alx@kernel.org>
>
> (Or maybe I should say
>
> Co-developed-by: Alejandro Colomar <alx@kernel.org>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>

I will add it.
Thanks for your help.

-- 
Regards
Yafang

[PATCH v8 7/8] net: Replace strcpy() with strscpy()

[Yafang Shao]

To prevent errors from occurring when the src string is longer than the dst
string in strcpy(), we should use strscpy() instead. This approach
also facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
---
 net/ipv6/ndisc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index b8eec1b6cc2c..cf7c36463b33 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -1944,7 +1944,7 @@ static void ndisc_warn_deprecated_sysctl(const struct ctl_table *ctl,
 	static char warncomm[TASK_COMM_LEN];
 	static int warned;
 	if (strcmp(warncomm, current->comm) && warned < 5) {
-		strcpy(warncomm, current->comm);
+		strscpy(warncomm, current->comm);
 		pr_warn("process `%s' is using deprecated sysctl (%s) net.ipv6.neigh.%s.%s - use net.ipv6.neigh.%s.%s_ms instead\n",
 			warncomm, func,
 			dev_name, ctl->procname,
-- 
2.43.5

Re: [PATCH v8 7/8] net: Replace strcpy() with strscpy()

[Justin Stitt]

Hi,

On Wed, Aug 28, 2024 at 11:03:20AM GMT, Yafang Shao wrote:
> To prevent errors from occurring when the src string is longer than the dst
> string in strcpy(), we should use strscpy() instead. This approach
> also facilitates future extensions to the task comm.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: David Ahern <dsahern@kernel.org>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/ipv6/ndisc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index b8eec1b6cc2c..cf7c36463b33 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1944,7 +1944,7 @@ static void ndisc_warn_deprecated_sysctl(const struct ctl_table *ctl,
>  	static char warncomm[TASK_COMM_LEN];
>  	static int warned;
>  	if (strcmp(warncomm, current->comm) && warned < 5) {
> -		strcpy(warncomm, current->comm);
> +		strscpy(warncomm, current->comm);
>  		pr_warn("process `%s' is using deprecated sysctl (%s) net.ipv6.neigh.%s.%s - use net.ipv6.neigh.%s.%s_ms instead\n",
>  			warncomm, func,
>  			dev_name, ctl->procname,
> -- 
> 2.43.5
> 
>

Reviewed-by: Justin Stitt <justinstitt@google.com>

Thanks
Justin

[PATCH v8 8/8] drm: Replace strcpy() with strscpy()

[Yafang Shao]

To prevent erros from occurring when the src string is longer than the
dst string in strcpy(), we should use strscpy() instead. This
approach also facilitates future extensions to the task comm.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@gmail.com>
---
 drivers/gpu/drm/drm_framebuffer.c     | 2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 888aadb6a4ac..2d6993539474 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -868,7 +868,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 	INIT_LIST_HEAD(&fb->filp_head);
 
 	fb->funcs = funcs;
-	strcpy(fb->comm, current->comm);
+	strscpy(fb->comm, current->comm);
 
 	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
 				    false, drm_framebuffer_free);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 96c6cafd5b9e..afa9dae39378 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -1412,7 +1412,7 @@ static bool record_context(struct i915_gem_context_coredump *e,
 	rcu_read_lock();
 	task = pid_task(ctx->pid, PIDTYPE_PID);
 	if (task) {
-		strcpy(e->comm, task->comm);
+		strscpy(e->comm, task->comm);
 		e->pid = task->pid;
 	}
 	rcu_read_unlock();
-- 
2.43.5

Re: [PATCH v8 8/8] drm: Replace strcpy() with strscpy()

[Justin Stitt]

Hi,

On Wed, Aug 28, 2024 at 11:03:21AM GMT, Yafang Shao wrote:
> To prevent erros from occurring when the src string is longer than the
> dst string in strcpy(), we should use strscpy() instead. This
> approach also facilitates future extensions to the task comm.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c     | 2 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> index 888aadb6a4ac..2d6993539474 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -868,7 +868,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  	INIT_LIST_HEAD(&fb->filp_head);
>  
>  	fb->funcs = funcs;
> -	strcpy(fb->comm, current->comm);
> +	strscpy(fb->comm, current->comm);
>  
>  	ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
>  				    false, drm_framebuffer_free);
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c

There are other strcpy() in this file but it seems all control paths to
the copies themselves stem from string literals, so it is probably fine
not to also change those ones. But, if a v9 is required and you're
feeling up to it, we should probably replace them too, as per [1].


> index 96c6cafd5b9e..afa9dae39378 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1412,7 +1412,7 @@ static bool record_context(struct i915_gem_context_coredump *e,
>  	rcu_read_lock();
>  	task = pid_task(ctx->pid, PIDTYPE_PID);
>  	if (task) {
> -		strcpy(e->comm, task->comm);
> +		strscpy(e->comm, task->comm);
>  		e->pid = task->pid;
>  	}
>  	rcu_read_unlock();
> -- 
> 2.43.5
> 
>


Reviewed-by: Justin Stitt <justinstitt@google.com>

[1]: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy

Thanks
Justin

Re: [PATCH v8 8/8] drm: Replace strcpy() with strscpy()

[Yafang Shao]

On Fri, Sep 13, 2024 at 5:28 AM Justin Stitt <justinstitt@google.com> wrote:
>
> Hi,
>
> On Wed, Aug 28, 2024 at 11:03:21AM GMT, Yafang Shao wrote:
> > To prevent erros from occurring when the src string is longer than the
> > dst string in strcpy(), we should use strscpy() instead. This
> > approach also facilitates future extensions to the task comm.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_framebuffer.c     | 2 +-
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
> > index 888aadb6a4ac..2d6993539474 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -868,7 +868,7 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
> >       INIT_LIST_HEAD(&fb->filp_head);
> >
> >       fb->funcs = funcs;
> > -     strcpy(fb->comm, current->comm);
> > +     strscpy(fb->comm, current->comm);
> >
> >       ret = __drm_mode_object_add(dev, &fb->base, DRM_MODE_OBJECT_FB,
> >                                   false, drm_framebuffer_free);
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>
> There are other strcpy() in this file but it seems all control paths to
> the copies themselves stem from string literals, so it is probably fine
> not to also change those ones. But, if a v9 is required and you're
> feeling up to it, we should probably replace them too, as per [1].

will change them in the next version.
Thanks for your suggestion.

>
>
> > index 96c6cafd5b9e..afa9dae39378 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -1412,7 +1412,7 @@ static bool record_context(struct i915_gem_context_coredump *e,
> >       rcu_read_lock();
> >       task = pid_task(ctx->pid, PIDTYPE_PID);
> >       if (task) {
> > -             strcpy(e->comm, task->comm);
> > +             strscpy(e->comm, task->comm);
> >               e->pid = task->pid;
> >       }
> >       rcu_read_unlock();
> > --
> > 2.43.5
> >
> >
>
>
> Reviewed-by: Justin Stitt <justinstitt@google.com>
>
> [1]: https://www.kernel.org/doc/html/latest/process/deprecated.html#strcpy
>
> Thanks
> Justin



-- 
Regards
Yafang