All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] exec: avoid gcc-8 warning for get_task_comm
@ 2017-12-05 15:17 Arnd Bergmann
  2017-12-05 19:09 ` Kees Cook
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-12-05 15:17 UTC (permalink / raw)
  To: Alexander Viro, Ingo Molnar, Peter Zijlstra
  Cc: Arnd Bergmann, Kees Cook, Serge Hallyn, James Morris,
	Andrew Morton, Aleksa Sarai, Eric W. Biederman,
	Frederic Weisbecker, Thomas Gleixner, linux-fsdevel,
	linux-kernel

gcc-8 warns about using strncpy() with the source size as the limit:

fs/exec.c:1223:32: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess]

This is indeed slightly suspicious, as it protects us from source
arguments without NUL-termination, but does not guarantee that the
destination is terminated.

This keeps the strncpy() to ensure we have properly padded target buffer,
but ensures that we use the correct length, by passing the actual length
of the destination buffer as well as adding a build-time check to ensure
it is exactly TASK_COMM_LEN.  There are only 23 callsights which I all
reviewed to ensure this is currently the case. We could get away with
doing only the check or passing the right length, but it doesn't hurt
to do both.

Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/exec.c             | 7 +++----
 include/linux/sched.h | 6 +++++-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 6be2aa0ab26f..156f56acfe8e 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1216,15 +1216,14 @@ static int de_thread(struct task_struct *tsk)
 	return -EAGAIN;
 }
 
-char *get_task_comm(char *buf, struct task_struct *tsk)
+char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
 {
-	/* buf must be at least sizeof(tsk->comm) in size */
 	task_lock(tsk);
-	strncpy(buf, tsk->comm, sizeof(tsk->comm));
+	strncpy(buf, tsk->comm, buf_size);
 	task_unlock(tsk);
 	return buf;
 }
-EXPORT_SYMBOL_GPL(get_task_comm);
+EXPORT_SYMBOL_GPL(__get_task_comm);
 
 /*
  * These functions flushes out all traces of the currently running executable
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 21991d668d35..5124ba709830 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1503,7 +1503,11 @@ 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, struct task_struct *tsk);
+extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
+#define get_task_comm(buf, tsk) ({			\
+	BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);	\
+	__get_task_comm(buf, sizeof(buf), tsk);		\
+})
 
 #ifdef CONFIG_SMP
 void scheduler_ipi(void);
-- 
2.9.0

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] exec: avoid gcc-8 warning for get_task_comm
  2017-12-05 15:17 [PATCH v2] exec: avoid gcc-8 warning for get_task_comm Arnd Bergmann
@ 2017-12-05 19:09 ` Kees Cook
  2017-12-06 10:11 ` David Laight
  2017-12-06 17:49 ` Ingo Molnar
  2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2017-12-05 19:09 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Alexander Viro, Ingo Molnar, Peter Zijlstra, Serge Hallyn,
	James Morris, Andrew Morton, Aleksa Sarai, Eric W. Biederman,
	Frederic Weisbecker, Thomas Gleixner, linux-fsdevel, LKML

On Tue, Dec 5, 2017 at 7:17 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> gcc-8 warns about using strncpy() with the source size as the limit:
>
> fs/exec.c:1223:32: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess]
>
> This is indeed slightly suspicious, as it protects us from source
> arguments without NUL-termination, but does not guarantee that the
> destination is terminated.
>
> This keeps the strncpy() to ensure we have properly padded target buffer,
> but ensures that we use the correct length, by passing the actual length
> of the destination buffer as well as adding a build-time check to ensure
> it is exactly TASK_COMM_LEN.  There are only 23 callsights which I all
> reviewed to ensure this is currently the case. We could get away with
> doing only the check or passing the right length, but it doesn't hurt
> to do both.
>
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Awesome. Thanks for checking them all!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  fs/exec.c             | 7 +++----
>  include/linux/sched.h | 6 +++++-
>  2 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 6be2aa0ab26f..156f56acfe8e 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1216,15 +1216,14 @@ static int de_thread(struct task_struct *tsk)
>         return -EAGAIN;
>  }
>
> -char *get_task_comm(char *buf, struct task_struct *tsk)
> +char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
>  {
> -       /* buf must be at least sizeof(tsk->comm) in size */
>         task_lock(tsk);
> -       strncpy(buf, tsk->comm, sizeof(tsk->comm));
> +       strncpy(buf, tsk->comm, buf_size);
>         task_unlock(tsk);
>         return buf;
>  }
> -EXPORT_SYMBOL_GPL(get_task_comm);
> +EXPORT_SYMBOL_GPL(__get_task_comm);
>
>  /*
>   * These functions flushes out all traces of the currently running executable
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 21991d668d35..5124ba709830 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1503,7 +1503,11 @@ 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, struct task_struct *tsk);
> +extern char *__get_task_comm(char *to, size_t len, struct task_struct *tsk);
> +#define get_task_comm(buf, tsk) ({                     \
> +       BUILD_BUG_ON(sizeof(buf) != TASK_COMM_LEN);     \
> +       __get_task_comm(buf, sizeof(buf), tsk);         \
> +})
>
>  #ifdef CONFIG_SMP
>  void scheduler_ipi(void);
> --
> 2.9.0
>



-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v2] exec: avoid gcc-8 warning for get_task_comm
  2017-12-05 15:17 [PATCH v2] exec: avoid gcc-8 warning for get_task_comm Arnd Bergmann
  2017-12-05 19:09 ` Kees Cook
@ 2017-12-06 10:11 ` David Laight
  2017-12-06 17:49 ` Ingo Molnar
  2 siblings, 0 replies; 6+ messages in thread
From: David Laight @ 2017-12-06 10:11 UTC (permalink / raw)
  To: 'Arnd Bergmann', Alexander Viro, Ingo Molnar, Peter Zijlstra
  Cc: Kees Cook, Serge Hallyn, James Morris, Andrew Morton,
	Aleksa Sarai, Eric W. Biederman, Frederic Weisbecker,
	Thomas Gleixner, linux-fsdevel, linux-kernel

From: Arnd Bergmann
> Sent: 05 December 2017 15:17
> gcc-8 warns about using strncpy() with the source size as the limit:

Hmmm... Someone 'fixed' some of those in the NetBSD tree a few years back.
There was one place where the code was correct.
A potentially unterminated string was being copied into a buffer that was
one byte longer.

	David

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] exec: avoid gcc-8 warning for get_task_comm
  2017-12-05 15:17 [PATCH v2] exec: avoid gcc-8 warning for get_task_comm Arnd Bergmann
  2017-12-05 19:09 ` Kees Cook
  2017-12-06 10:11 ` David Laight
@ 2017-12-06 17:49 ` Ingo Molnar
  2017-12-06 23:09   ` Kees Cook
  2 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2017-12-06 17:49 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Alexander Viro, Ingo Molnar, Peter Zijlstra, Kees Cook,
	Serge Hallyn, James Morris, Andrew Morton, Aleksa Sarai,
	Eric W. Biederman, Frederic Weisbecker, Thomas Gleixner,
	linux-fsdevel, linux-kernel


* Arnd Bergmann <arnd@arndb.de> wrote:

> gcc-8 warns about using strncpy() with the source size as the limit:
> 
> fs/exec.c:1223:32: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess]
> 
> This is indeed slightly suspicious, as it protects us from source
> arguments without NUL-termination, but does not guarantee that the
> destination is terminated.
> 
> This keeps the strncpy() to ensure we have properly padded target buffer,
> but ensures that we use the correct length, by passing the actual length
> of the destination buffer as well as adding a build-time check to ensure
> it is exactly TASK_COMM_LEN.  There are only 23 callsights which I all
> reviewed to ensure this is currently the case. We could get away with
> doing only the check or passing the right length, but it doesn't hurt
> to do both.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Looks useful.

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] exec: avoid gcc-8 warning for get_task_comm
  2017-12-06 17:49 ` Ingo Molnar
@ 2017-12-06 23:09   ` Kees Cook
  2017-12-06 23:29     ` Andrew Morton
  0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2017-12-06 23:09 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Arnd Bergmann, Alexander Viro, Ingo Molnar, Peter Zijlstra,
	Serge Hallyn, James Morris, Andrew Morton, Aleksa Sarai,
	Eric W. Biederman, Frederic Weisbecker, Thomas Gleixner,
	linux-fsdevel, LKML

On Wed, Dec 6, 2017 at 9:49 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Arnd Bergmann <arnd@arndb.de> wrote:
>
>> gcc-8 warns about using strncpy() with the source size as the limit:
>>
>> fs/exec.c:1223:32: error: argument to 'sizeof' in 'strncpy' call is the same expression as the source; did you mean to use the size of the destination? [-Werror=sizeof-pointer-memaccess]
>>
>> This is indeed slightly suspicious, as it protects us from source
>> arguments without NUL-termination, but does not guarantee that the
>> destination is terminated.
>>
>> This keeps the strncpy() to ensure we have properly padded target buffer,
>> but ensures that we use the correct length, by passing the actual length
>> of the destination buffer as well as adding a build-time check to ensure
>> it is exactly TASK_COMM_LEN.  There are only 23 callsights which I all
>> reviewed to ensure this is currently the case. We could get away with
>> doing only the check or passing the right length, but it doesn't hurt
>> to do both.
>>
>> Suggested-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Looks useful.
>
> Acked-by: Ingo Molnar <mingo@kernel.org>

Ingo, can you take this into -tip, or should this go via -mm or some other tree?

-Kees

-- 
Kees Cook
Pixel Security

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] exec: avoid gcc-8 warning for get_task_comm
  2017-12-06 23:09   ` Kees Cook
@ 2017-12-06 23:29     ` Andrew Morton
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2017-12-06 23:29 UTC (permalink / raw)
  To: Kees Cook
  Cc: Ingo Molnar, Arnd Bergmann, Alexander Viro, Ingo Molnar,
	Peter Zijlstra, Serge Hallyn, James Morris, Aleksa Sarai,
	Eric W. Biederman, Frederic Weisbecker, Thomas Gleixner,
	linux-fsdevel, LKML

On Wed, 6 Dec 2017 15:09:20 -0800 Kees Cook <keescook@chromium.org> wrote:

> >> to do both.
> >>
> >> Suggested-by: Kees Cook <keescook@chromium.org>
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Looks useful.
> >
> > Acked-by: Ingo Molnar <mingo@kernel.org>
> 
> Ingo, can you take this into -tip, or should this go via -mm or some other tree?

I have it queued for 4.15.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-12-06 23:29 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-05 15:17 [PATCH v2] exec: avoid gcc-8 warning for get_task_comm Arnd Bergmann
2017-12-05 19:09 ` Kees Cook
2017-12-06 10:11 ` David Laight
2017-12-06 17:49 ` Ingo Molnar
2017-12-06 23:09   ` Kees Cook
2017-12-06 23:29     ` Andrew Morton

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.