| Message ID | 20171204144916.453471-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 4, 2017 at 6:49 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 changes it to strlcpy with a hardcoded length, to guarantee > a properly terminated string. Since we already use strlcpy() for > __set_task_comm(), the source should always be terminated properly, It's terminated but it's not padded. This change _might_ result in leaving a string buffer uninitialized (and if that buffer is passed as a whole instead of just its strlen(), this could be a problem: proc_comm_connector() e.g. may suffer from this, I haven't checked closely). > so this patch won't change the behavior, but make it a bit more robust. I would rather we change get_task_comm() to include the size of buf... maybe just convert it to a macro: -char *get_task_comm(char *buf, struct task_struct *tsk) +char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) ... #define get_task_comm(buf, tsk) __get_task_comm(buf, sizeof(buf), tsk) ? That will only work for preallocated arrays though. Maybe a build bug when sizeof(buf) != TASK_COMM_LEN ? I think the strncpy() should stay, but for other conversions where NULL-padding isn't an issue, strscpy() is likely preferred to strlcpy() to avoid the potential "overread" due to the strlen() call in strlcpy(). -Kees > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/exec.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 6be2aa0ab26f..3e8012afe440 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1218,9 +1218,9 @@ static int de_thread(struct task_struct *tsk) > > char *get_task_comm(char *buf, struct task_struct *tsk) > { > - /* buf must be at least sizeof(tsk->comm) in size */ > + /* buf must be at least TASK_COMM_LEN in size */ > task_lock(tsk); > - strncpy(buf, tsk->comm, sizeof(tsk->comm)); > + strlcpy(buf, tsk->comm, TASK_COMM_LEN); > task_unlock(tsk); > return buf; > } > -- > 2.9.0 >
On Mon, Dec 4, 2017 at 7:37 PM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Dec 4, 2017 at 6:49 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 changes it to strlcpy with a hardcoded length, to guarantee >> a properly terminated string. Since we already use strlcpy() for >> __set_task_comm(), the source should always be terminated properly, > > It's terminated but it's not padded. This change _might_ result in > leaving a string buffer uninitialized (and if that buffer is passed as > a whole instead of just its strlen(), this could be a problem: > proc_comm_connector() e.g. may suffer from this, I haven't checked > closely). > >> so this patch won't change the behavior, but make it a bit more robust. > > I would rather we change get_task_comm() to include the size of buf... > maybe just convert it to a macro: > > -char *get_task_comm(char *buf, struct task_struct *tsk) > +char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) > > ... > > #define get_task_comm(buf, tsk) __get_task_comm(buf, sizeof(buf), tsk) > > ? > > That will only work for preallocated arrays though. Maybe a build bug > when sizeof(buf) != TASK_COMM_LEN ? Interesting idea, I'll give that a try. > I think the strncpy() should stay, but for other conversions where > NULL-padding isn't an issue, strscpy() is likely preferred to > strlcpy() to avoid the potential "overread" due to the strlen() call > in strlcpy(). Ok, I'll keep that in mind if I do more of those conversions. gcc-8 has started warning about over 100 drivers that use strncpy() incorrectly, and I suppose a lot of them should actually use strscpy(). Arnd
diff --git a/fs/exec.c b/fs/exec.c index 6be2aa0ab26f..3e8012afe440 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1218,9 +1218,9 @@ static int de_thread(struct task_struct *tsk) char *get_task_comm(char *buf, struct task_struct *tsk) { - /* buf must be at least sizeof(tsk->comm) in size */ + /* buf must be at least TASK_COMM_LEN in size */ task_lock(tsk); - strncpy(buf, tsk->comm, sizeof(tsk->comm)); + strlcpy(buf, tsk->comm, TASK_COMM_LEN); task_unlock(tsk); return buf; }
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 changes it to strlcpy with a hardcoded length, to guarantee a properly terminated string. Since we already use strlcpy() for __set_task_comm(), the source should always be terminated properly, so this patch won't change the behavior, but make it a bit more robust. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/exec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)