2021-08-17 10:28:52

by Len Baker

Subject: [PATCH] CIFS: Fix a potencially linear read overflow

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.

Also, the strnlen() call does not avoid the read overflow in the strlcpy
function when a not NUL-terminated string is passed.

So, replace this block by a call to kstrndup() that avoids this type of
overflow and does the same.

Fixes: 066ce6899484d ("cifs: rename cifs_strlcpy_to_host and make it use new functions")
Signed-off-by: Len Baker <[email protected]>
---
fs/cifs/cifs_unicode.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 9bd03a231032..171ad8b42107 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -358,14 +358,9 @@ cifs_strndup_from_utf16(const char *src, const int maxlen,
if (!dst)
return NULL;
cifs_from_utf16(dst, (__le16 *) src, len, maxlen, codepage,
- NO_MAP_UNI_RSVD);
+ NO_MAP_UNI_RSVD);
} else {
- len = strnlen(src, maxlen);
- len++;
- dst = kmalloc(len, GFP_KERNEL);
- if (!dst)
- return NULL;
- strlcpy(dst, src, len);
+ dst = kstrndup(src, maxlen, GFP_KERNEL);
}

return dst;
--
2.25.1

2021-08-18 00:32:36

by Paulo Alcantara

Subject: Re: [PATCH] CIFS: Fix a potencially linear read overflow

Len Baker <[email protected]> writes:

> 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.
>
> Also, the strnlen() call does not avoid the read overflow in the strlcpy
> function when a not NUL-terminated string is passed.
>
> So, replace this block by a call to kstrndup() that avoids this type of
> overflow and does the same.
>
> Fixes: 066ce6899484d ("cifs: rename cifs_strlcpy_to_host and make it use new functions")
> Signed-off-by: Len Baker <[email protected]>
> ---
> fs/cifs/cifs_unicode.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Paulo Alcantara (SUSE) <[email protected]>

2021-08-18 02:51:17

by Steve French

Subject: Re: [PATCH] CIFS: Fix a potencially linear read overflow

tentatively merged into cifs-2.6.git for-next pending testing

On Tue, Aug 17, 2021 at 7:29 PM Paulo Alcantara <[email protected]> wrote:
>
> Len Baker <[email protected]> writes:
>
> > 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.
> >
> > Also, the strnlen() call does not avoid the read overflow in the strlcpy
> > function when a not NUL-terminated string is passed.
> >
> > So, replace this block by a call to kstrndup() that avoids this type of
> > overflow and does the same.
> >
> > Fixes: 066ce6899484d ("cifs: rename cifs_strlcpy_to_host and make it use new functions")
> > Signed-off-by: Len Baker <[email protected]>
> > ---
> > fs/cifs/cifs_unicode.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Reviewed-by: Paulo Alcantara (SUSE) <[email protected]>



--
Thanks,

Steve

2021-08-18 13:23:49

by Jeffrey Layton

Subject: Re: [PATCH] CIFS: Fix a potencially linear read overflow

On Tue, 2021-08-17 at 12:27 +0200, Len Baker 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.
>
> Also, the strnlen() call does not avoid the read overflow in the strlcpy
> function when a not NUL-terminated string is passed.
>
> So, replace this block by a call to kstrndup() that avoids this type of
> overflow and does the same.
>
> Fixes: 066ce6899484d ("cifs: rename cifs_strlcpy_to_host and make it use new functions")
> Signed-off-by: Len Baker <[email protected]>
> ---
> fs/cifs/cifs_unicode.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 9bd03a231032..171ad8b42107 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -358,14 +358,9 @@ cifs_strndup_from_utf16(const char *src, const int maxlen,
> if (!dst)
> return NULL;
> cifs_from_utf16(dst, (__le16 *) src, len, maxlen, codepage,
> - NO_MAP_UNI_RSVD);
> + NO_MAP_UNI_RSVD);
> } else {
> - len = strnlen(src, maxlen);
> - len++;
> - dst = kmalloc(len, GFP_KERNEL);
> - if (!dst)
> - return NULL;
> - strlcpy(dst, src, len);
> + dst = kstrndup(src, maxlen, GFP_KERNEL);
> }
>
> return dst;
> --
> 2.25.1
>

Reviewed-by: Jeff Layton <[email protected]>

2021-08-20 06:04:16

by Steve French

Subject: Re: [PATCH] CIFS: Fix a potencially linear read overflow

Added RB and repushed to cifs-2.6.git for-next

On Wed, Aug 18, 2021 at 8:22 AM Jeff Layton <[email protected]> wrote:
>
> On Tue, 2021-08-17 at 12:27 +0200, Len Baker 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.
> >
> > Also, the strnlen() call does not avoid the read overflow in the strlcpy
> > function when a not NUL-terminated string is passed.
> >
> > So, replace this block by a call to kstrndup() that avoids this type of
> > overflow and does the same.
> >
> > Fixes: 066ce6899484d ("cifs: rename cifs_strlcpy_to_host and make it use new functions")
> > Signed-off-by: Len Baker <[email protected]>
> > ---
> > fs/cifs/cifs_unicode.c | 9 ++-------
> > 1 file changed, 2 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> > index 9bd03a231032..171ad8b42107 100644
> > --- a/fs/cifs/cifs_unicode.c
> > +++ b/fs/cifs/cifs_unicode.c
> > @@ -358,14 +358,9 @@ cifs_strndup_from_utf16(const char *src, const int maxlen,
> > if (!dst)
> > return NULL;
> > cifs_from_utf16(dst, (__le16 *) src, len, maxlen, codepage,
> > - NO_MAP_UNI_RSVD);
> > + NO_MAP_UNI_RSVD);
> > } else {
> > - len = strnlen(src, maxlen);
> > - len++;
> > - dst = kmalloc(len, GFP_KERNEL);
> > - if (!dst)
> > - return NULL;
> > - strlcpy(dst, src, len);
> > + dst = kstrndup(src, maxlen, GFP_KERNEL);
> > }
> >
> > return dst;
> > --
> > 2.25.1
> >
>
> Reviewed-by: Jeff Layton <[email protected]>
>


--
Thanks,

Steve