diff mbox series

[v6,02/12] fs/exec: make __get_task_comm always get a nul terminated string

Message ID 20211025083315.4752-3-laoar.shao@gmail.com (mailing list archive)
State Superseded
Headers show
Series extend task comm from 16 to 24 | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-next success VM_Test
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf success VM_Test
bpf/vmtest-bpf-PR success PR summary

Commit Message

Yafang Shao Oct. 25, 2021, 8:33 a.m. UTC
If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
will be without null ternimator, that may cause problem. We can make sure
the buffer size not smaller than comm at the callsite to avoid that
problem, but there may be callsite that we can't easily change.

Using strscpy_pad() instead of strncpy() in __get_task_comm() can make
the string always nul ternimated.

Suggested-by: Kees Cook <keescook@chromium.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Kees Cook <keescook@chromium.org>
Cc: Petr Mladek <pmladek@suse.com>
---
 fs/exec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Kees Cook Oct. 25, 2021, 9:08 p.m. UTC | #1
On Mon, Oct 25, 2021 at 08:33:05AM +0000, Yafang Shao wrote:
> If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
> will be without null ternimator, that may cause problem. We can make sure
> the buffer size not smaller than comm at the callsite to avoid that
> problem, but there may be callsite that we can't easily change.
> 
> Using strscpy_pad() instead of strncpy() in __get_task_comm() can make
> the string always nul ternimated.
> 
> Suggested-by: Kees Cook <keescook@chromium.org>
> Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Petr Mladek <pmladek@suse.com>
> ---
>  fs/exec.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 404156b5b314..bf2a7a91eeea 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1209,7 +1209,8 @@ static int unshare_sighand(struct task_struct *me)
>  char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
>  {
>  	task_lock(tsk);
> -	strncpy(buf, tsk->comm, buf_size);
> +	/* The copied value is always null terminated */

This may could say "always NUL terminated and zero-padded"

> +	strscpy_pad(buf, tsk->comm, buf_size);
>  	task_unlock(tsk);
>  	return buf;
>  }
> -- 
> 2.17.1
> 

But for the replacement with strscpy_pad(), yes please:

Reviewed-by: Kees Cook <keescook@chromium.org>
Yafang Shao Oct. 26, 2021, 1:49 a.m. UTC | #2
On Tue, Oct 26, 2021 at 5:08 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Oct 25, 2021 at 08:33:05AM +0000, Yafang Shao wrote:
> > If the dest buffer size is smaller than sizeof(tsk->comm), the buffer
> > will be without null ternimator, that may cause problem. We can make sure
> > the buffer size not smaller than comm at the callsite to avoid that
> > problem, but there may be callsite that we can't easily change.
> >
> > Using strscpy_pad() instead of strncpy() in __get_task_comm() can make
> > the string always nul ternimated.
> >
> > Suggested-by: Kees Cook <keescook@chromium.org>
> > Suggested-by: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Al Viro <viro@zeniv.linux.org.uk>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Petr Mladek <pmladek@suse.com>
> > ---
> >  fs/exec.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 404156b5b314..bf2a7a91eeea 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1209,7 +1209,8 @@ static int unshare_sighand(struct task_struct *me)
> >  char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
> >  {
> >       task_lock(tsk);
> > -     strncpy(buf, tsk->comm, buf_size);
> > +     /* The copied value is always null terminated */
>
> This may could say "always NUL terminated and zero-padded"
>

Sure. I will change it.

> > +     strscpy_pad(buf, tsk->comm, buf_size);
> >       task_unlock(tsk);
> >       return buf;
> >  }
> > --
> > 2.17.1
> >
>
> But for the replacement with strscpy_pad(), yes please:
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
>
> --
> Kees Cook
diff mbox series

Patch

diff --git a/fs/exec.c b/fs/exec.c
index 404156b5b314..bf2a7a91eeea 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1209,7 +1209,8 @@  static int unshare_sighand(struct task_struct *me)
 char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk)
 {
 	task_lock(tsk);
-	strncpy(buf, tsk->comm, buf_size);
+	/* The copied value is always null terminated */
+	strscpy_pad(buf, tsk->comm, buf_size);
 	task_unlock(tsk);
 	return buf;
 }