| Message ID | 20230512155749.1356958-1-azeemshaikh38@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 8ca25e00cf817b635f4e80d59b6d07686d74eff0 |
| Headers | show |
| Series | NFS: Prefer strscpy over strlcpy calls | expand |
On Fri, May 12, 2023 at 03:57:49PM +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > Check for strscpy()'s return value of -E2BIG on truncate for safe > replacement with strlcpy(). > > This is part of a tree-wide cleanup to remove the strlcpy() function > entirely from the kernel [2]. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> > --- > Note to reviewers: the one case where this patch would modify existing behavior > is when strlen(src)==destlen==0. Current behavior returns 0, with this patch it > would return -1. > > Not sure what the implication of this updated behavior would be, > so bringing it to your attention. I'm not sure either, but I would prefer non-terminated strings produce an error, which this change does. :) Reviewed-by: Kees Cook <keescook@chromium.org> -Kees > > fs/nfs/nfsroot.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c > index 620329b7e6ae..7600100ba26f 100644 > --- a/fs/nfs/nfsroot.c > +++ b/fs/nfs/nfsroot.c > @@ -164,7 +164,7 @@ __setup("nfsroot=", nfs_root_setup); > static int __init root_nfs_copy(char *dest, const char *src, > const size_t destlen) > { > - if (strlcpy(dest, src, destlen) > destlen) > + if (strscpy(dest, src, destlen) == -E2BIG) > return -1; > return 0; > } > -- > 2.40.1.606.ga4b1b128d6-goog > >
> > --- > > Note to reviewers: the one case where this patch would modify existing behavior > > is when strlen(src)==destlen==0. Current behavior returns 0, with this patch it > > would return -1. > > > > Not sure what the implication of this updated behavior would be, > > so bringing it to your attention. > > I'm not sure either, but I would prefer non-terminated strings produce > an error, which this change does. :) > > Reviewed-by: Kees Cook <keescook@chromium.org> > Friendly ping.
On Fri, 12 May 2023 15:57:49 +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > Check for strscpy()'s return value of -E2BIG on truncate for safe > replacement with strlcpy(). > > [...] Applied to for-next/hardening, thanks! [1/1] NFS: Prefer strscpy over strlcpy calls https://git.kernel.org/kees/c/8ca25e00cf81
diff --git a/fs/nfs/nfsroot.c b/fs/nfs/nfsroot.c index 620329b7e6ae..7600100ba26f 100644 --- a/fs/nfs/nfsroot.c +++ b/fs/nfs/nfsroot.c @@ -164,7 +164,7 @@ __setup("nfsroot=", nfs_root_setup); static int __init root_nfs_copy(char *dest, const char *src, const size_t destlen) { - if (strlcpy(dest, src, destlen) > destlen) + if (strscpy(dest, src, destlen) == -E2BIG) return -1; return 0; }
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. Check for strscpy()'s return value of -E2BIG on truncate for safe replacement with strlcpy(). This is part of a tree-wide cleanup to remove the strlcpy() function entirely from the kernel [2]. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> --- Note to reviewers: the one case where this patch would modify existing behavior is when strlen(src)==destlen==0. Current behavior returns 0, with this patch it would return -1. Not sure what the implication of this updated behavior would be, so bringing it to your attention. fs/nfs/nfsroot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)