Make sure the string set to task comm is always nul terminated.
Signed-off-by: Yafang Shao <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
Cc: Mathieu Desnoyers <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Andrii Nakryiko <[email protected]>
Cc: Michal Miroslaw <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Steven Rostedt <[email protected]>
Cc: Matthew Wilcox <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Al Viro <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Petr Mladek <[email protected]>
---
fs/exec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/exec.c b/fs/exec.c
index a098c133d8d7..404156b5b314 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
{
task_lock(tsk);
trace_task_rename(tsk, buf);
- strlcpy(tsk->comm, buf, sizeof(tsk->comm));
+ strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
task_unlock(tsk);
perf_event_comm(tsk, exec);
}
--
2.17.1
On 08.11.21 09:38, Yafang Shao wrote:
> Make sure the string set to task comm is always nul terminated.
>
strlcpy: "the result is always a valid NUL-terminated string that fits
in the buffer"
The only difference seems to be that strscpy_pad() pads the remainder
with zeroes.
Is this description correct and I am missing something important?
> Signed-off-by: Yafang Shao <[email protected]>
> Reviewed-by: Kees Cook <[email protected]>
> Cc: Mathieu Desnoyers <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Andrii Nakryiko <[email protected]>
> Cc: Michal Miroslaw <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: Steven Rostedt <[email protected]>
> Cc: Matthew Wilcox <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Al Viro <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Petr Mladek <[email protected]>
> ---
> fs/exec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index a098c133d8d7..404156b5b314 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
> {
> task_lock(tsk);
> trace_task_rename(tsk, buf);
> - strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> + strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
> task_unlock(tsk);
> perf_event_comm(tsk, exec);
> }
>
--
Thanks,
David / dhildenb
On Wed, Nov 10, 2021 at 4:28 PM David Hildenbrand <[email protected]> wrote:
>
> On 08.11.21 09:38, Yafang Shao wrote:
> > Make sure the string set to task comm is always nul terminated.
> >
>
> strlcpy: "the result is always a valid NUL-terminated string that fits
> in the buffer"
>
> The only difference seems to be that strscpy_pad() pads the remainder
> with zeroes.
>
> Is this description correct and I am missing something important?
>
In a earlier version [1], the checkpatch.py found a warning:
WARNING: Prefer strscpy over strlcpy - see:
https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
So I replaced strlcpy() with strscpy() to fix this warning.
And then in v5[2], the strscpy() was replaced with strscpy_pad() to
make sure there's no garbade data and also make get_task_comm() be
consistent with get_task_comm().
This commit log didn't clearly describe the historical changes. So I
think we can update the commit log and subject with:
Subject: use strscpy_pad with strlcpy in __set_task_comm
Commit log:
strlcpy is not suggested to use by the checkpatch.pl, so we'd better
recplace it with strscpy.
To avoid leaving garbage data and be consistent with the usage in
__get_task_comm(), the strscpy_pad is used here.
WDYT?
[1]. https://lore.kernel.org/lkml/[email protected]/
[2]. https://lore.kernel.org/lkml/[email protected]/
> > Signed-off-by: Yafang Shao <[email protected]>
> > Reviewed-by: Kees Cook <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Andrii Nakryiko <[email protected]>
> > Cc: Michal Miroslaw <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > ---
> > fs/exec.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index a098c133d8d7..404156b5b314 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
> > {
> > task_lock(tsk);
> > trace_task_rename(tsk, buf);
> > - strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> > + strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
> > task_unlock(tsk);
> > perf_event_comm(tsk, exec);
> > }
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>
--
Thanks
Yafang
On Wed, Nov 10, 2021 at 09:28:12AM +0100, David Hildenbrand wrote:
> On 08.11.21 09:38, Yafang Shao wrote:
> > Make sure the string set to task comm is always nul terminated.
> >
>
> strlcpy: "the result is always a valid NUL-terminated string that fits
> in the buffer"
>
> The only difference seems to be that strscpy_pad() pads the remainder
> with zeroes.
>
> Is this description correct and I am missing something important?
Yes, this makes sure it's zero padded just to be robust against full
tsk->comm copies that got noticed in other places.
The only other change is that we want to remove strlcpy() from the
kernel generally since it can trigger out-of-bound reads on the source
string[1].
So, in this case, the most robust version is to use strscpy_pad().
-Kees
[1] https://github.com/KSPP/linux/issues/89
>
> > Signed-off-by: Yafang Shao <[email protected]>
> > Reviewed-by: Kees Cook <[email protected]>
> > Cc: Mathieu Desnoyers <[email protected]>
> > Cc: Arnaldo Carvalho de Melo <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Andrii Nakryiko <[email protected]>
> > Cc: Michal Miroslaw <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > Cc: Steven Rostedt <[email protected]>
> > Cc: Matthew Wilcox <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Al Viro <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > ---
> > fs/exec.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index a098c133d8d7..404156b5b314 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1224,7 +1224,7 @@ void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
> > {
> > task_lock(tsk);
> > trace_task_rename(tsk, buf);
> > - strlcpy(tsk->comm, buf, sizeof(tsk->comm));
> > + strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
> > task_unlock(tsk);
> > perf_event_comm(tsk, exec);
> > }
> >
>
>
> --
> Thanks,
>
> David / dhildenb
>
--
Kees Cook
On 10.11.21 10:05, Yafang Shao wrote:
> On Wed, Nov 10, 2021 at 4:28 PM David Hildenbrand <[email protected]> wrote:
>>
>> On 08.11.21 09:38, Yafang Shao wrote:
>>> Make sure the string set to task comm is always nul terminated.
>>>
>>
>> strlcpy: "the result is always a valid NUL-terminated string that fits
>> in the buffer"
>>
>> The only difference seems to be that strscpy_pad() pads the remainder
>> with zeroes.
>>
>> Is this description correct and I am missing something important?
>>
>
> In a earlier version [1], the checkpatch.py found a warning:
> WARNING: Prefer strscpy over strlcpy - see:
> https://lore.kernel.org/r/CAHk-=wgfRnXz0W3D37d01q3JFkr_i_uTL=V6A6G1oUZcprmknw@mail.gmail.com/
> So I replaced strlcpy() with strscpy() to fix this warning.
> And then in v5[2], the strscpy() was replaced with strscpy_pad() to
> make sure there's no garbade data and also make get_task_comm() be
> consistent with get_task_comm().
>
> This commit log didn't clearly describe the historical changes. So I
> think we can update the commit log and subject with:
>
> Subject: use strscpy_pad with strlcpy in __set_task_comm
> Commit log:
> strlcpy is not suggested to use by the checkpatch.pl, so we'd better
> recplace it with strscpy.
> To avoid leaving garbage data and be consistent with the usage in
> __get_task_comm(), the strscpy_pad is used here.
>
> WDYT?
Yes, that makes it clearer what this patch actually does :)
With the subject+description changed
Reviewed-by: David Hildenbrand <[email protected]>
Thanks!
--
Thanks,
David / dhildenb