[PATCH] lib/string: avoid reading beyond src buffer in strscpy

[PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Eryu Guan]

strscpy() tries to copy sizeof(unsigned long) bytes a time from src
to dest when possible, and stops the loop when 'max' is less than
sizeof(unsigned long). But it doesn't check if (src+res) goes beyond
src buffer and does out-of-bound access to the underlying memory.

KASAN reported global-out-of-bound bug when reading seccomp
actions_logged file in procfs:

  cat /proc/sys/kernel/seccomp/actions_logged

Because seccomp_names_from_actions_logged() is copying short strings
(less than sizeof(unsigned long)) to buffer 'names'. e.g.

  ret = strscpy(names, " ", size);

Fixed by capping the 'max' value according to the src buffer size,
to make sure we won't go beyond src buffer.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Metcalf <cmetcalf@ezchip.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Eryu Guan <eguan@redhat.com>
---
 lib/string.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/string.c b/lib/string.c
index 64a9e33f1daa..13a0147eea00 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -179,6 +179,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
 {
 	const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
 	size_t max = count;
+	size_t src_sz = strlen(src) + 1;
 	long res = 0;
 
 	if (count == 0)
@@ -200,6 +201,10 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
 		max = 0;
 #endif
 
+	/* avoid reading beyond src buffer */
+	if (max > src_sz)
+		max = src_sz;
+
 	while (max >= sizeof(unsigned long)) {
 		unsigned long c, data;
 
-- 
2.14.3

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Kees Cook]

On Thu, Dec 7, 2017 at 3:33 AM, Eryu Guan <eguan@redhat.com> wrote:
> strscpy() tries to copy sizeof(unsigned long) bytes a time from src
> to dest when possible, and stops the loop when 'max' is less than
> sizeof(unsigned long). But it doesn't check if (src+res) goes beyond
> src buffer and does out-of-bound access to the underlying memory.
>
> KASAN reported global-out-of-bound bug when reading seccomp
> actions_logged file in procfs:
>
>   cat /proc/sys/kernel/seccomp/actions_logged
>
> Because seccomp_names_from_actions_logged() is copying short strings
> (less than sizeof(unsigned long)) to buffer 'names'. e.g.
>
>   ret = strscpy(names, " ", size);

This is a false positive:
https://marc.info/?l=linux-kernel&m=150768944030805&w=2

Given that we keep getting these reports (this is the third), I wonder
if can adjust the seccomp code to work around the bug in KASAN...

> Fixed by capping the 'max' value according to the src buffer size,
> to make sure we won't go beyond src buffer.
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Chris Metcalf <cmetcalf@ezchip.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Eryu Guan <eguan@redhat.com>
> ---
>  lib/string.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/lib/string.c b/lib/string.c
> index 64a9e33f1daa..13a0147eea00 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -179,6 +179,7 @@ ssize_t strscpy(char *dest, const char *src, size_t count)
>  {
>         const struct word_at_a_time constants = WORD_AT_A_TIME_CONSTANTS;
>         size_t max = count;
> +       size_t src_sz = strlen(src) + 1;

NAK. The whole point of strscpy is to avoid over-reading the source
(see the comments above the function):

 * Preferred to strlcpy() since the API doesn't require reading memory
 * from the src string beyond the specified "count" bytes, and since
 * the return value is easier to error-check than strlcpy()'s.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Andrey Ryabinin]

On 12/07/2017 09:26 PM, Kees Cook wrote:
> On Thu, Dec 7, 2017 at 3:33 AM, Eryu Guan <eguan@redhat.com> wrote:
>> strscpy() tries to copy sizeof(unsigned long) bytes a time from src
>> to dest when possible, and stops the loop when 'max' is less than
>> sizeof(unsigned long). But it doesn't check if (src+res) goes beyond
>> src buffer and does out-of-bound access to the underlying memory.
>>
>> KASAN reported global-out-of-bound bug when reading seccomp
>> actions_logged file in procfs:
>>
>>   cat /proc/sys/kernel/seccomp/actions_logged
>>
>> Because seccomp_names_from_actions_logged() is copying short strings
>> (less than sizeof(unsigned long)) to buffer 'names'. e.g.
>>
>>   ret = strscpy(names, " ", size);
> 
> This is a false positive:
> https://marc.info/?l=linux-kernel&m=150768944030805&w=2
> 
> Given that we keep getting these reports (this is the third), I wonder
> if can adjust the seccomp code to work around the bug in KASAN...
> 

We should fix this in strscpy() somehow. We just need to decide how to do this.
Last time we haven't came to agreement.

So, possible solutions are:

1) Simply disable word-at-a-time optimization in strscpy(). I seriously doubt
that this optimization have noticeable performance impact in real workloads.

2) Apply patch https://lkml.kernel.org/r/20170718171523.32208-1-aryabinin@virtuozzo.com
It's a simple, minimally intrusive way to fix the bug.
However, the patch changes semantics/implementation of the strscpy() which is bad idea.
It basically means that we use one implementation of the strscpy() but test with KASAN another implementation.
For that reason patch wasn't liked by Linus.

3) Introduce read_once_partial_check() function and use it in strscpy().
read_once_partial_check() supposed to read one word, but KASAN will check only the first byte of the access.
Dmitry didn't like this. I'm also not fan of this solution, because we may not notice some real bugs, e.g.:

	char dst[8];
	char *src;
	
	src = kmalloc(5, GFP_KERNEL);
	memset(src, 0xff, 5);
	strscpy(dst, src, 8);

stscpy() will copy from 6 up to 8 bytes (exact size depends on what is stored in src[5-7])
from non-null terminated src. With read_once_partial_check() used in strscpy() KASAN will
not report such bug.


So, what it's gonna be? Let's finally decide something and deal with the problem.
My vote belongs to 1) or 2).

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Dmitry Vyukov]

On Fri, Dec 8, 2017 at 4:29 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> On 12/07/2017 09:26 PM, Kees Cook wrote:
>> On Thu, Dec 7, 2017 at 3:33 AM, Eryu Guan <eguan@redhat.com> wrote:
>>> strscpy() tries to copy sizeof(unsigned long) bytes a time from src
>>> to dest when possible, and stops the loop when 'max' is less than
>>> sizeof(unsigned long). But it doesn't check if (src+res) goes beyond
>>> src buffer and does out-of-bound access to the underlying memory.
>>>
>>> KASAN reported global-out-of-bound bug when reading seccomp
>>> actions_logged file in procfs:
>>>
>>>   cat /proc/sys/kernel/seccomp/actions_logged
>>>
>>> Because seccomp_names_from_actions_logged() is copying short strings
>>> (less than sizeof(unsigned long)) to buffer 'names'. e.g.
>>>
>>>   ret = strscpy(names, " ", size);
>>
>> This is a false positive:
>> https://marc.info/?l=linux-kernel&m=150768944030805&w=2
>>
>> Given that we keep getting these reports (this is the third), I wonder
>> if can adjust the seccomp code to work around the bug in KASAN...
>>
>
> We should fix this in strscpy() somehow. We just need to decide how to do this.
> Last time we haven't came to agreement.
>
> So, possible solutions are:
>
> 1) Simply disable word-at-a-time optimization in strscpy(). I seriously doubt
> that this optimization have noticeable performance impact in real workloads.
>
> 2) Apply patch https://lkml.kernel.org/r/20170718171523.32208-1-aryabinin@virtuozzo.com
> It's a simple, minimally intrusive way to fix the bug.
> However, the patch changes semantics/implementation of the strscpy() which is bad idea.
> It basically means that we use one implementation of the strscpy() but test with KASAN another implementation.
> For that reason patch wasn't liked by Linus.
>
> 3) Introduce read_once_partial_check() function and use it in strscpy().
> read_once_partial_check() supposed to read one word, but KASAN will check only the first byte of the access.
> Dmitry didn't like this. I'm also not fan of this solution, because we may not notice some real bugs, e.g.:
>
>         char dst[8];
>         char *src;
>
>         src = kmalloc(5, GFP_KERNEL);
>         memset(src, 0xff, 5);
>         strscpy(dst, src, 8);
>
> stscpy() will copy from 6 up to 8 bytes (exact size depends on what is stored in src[5-7])
> from non-null terminated src. With read_once_partial_check() used in strscpy() KASAN will
> not report such bug.
>
>
> So, what it's gonna be? Let's finally decide something and deal with the problem.
> My vote belongs to 1) or 2).


My vote is for 1) if we agree that the optimization is not worth it,
otherwise for 2).

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Kees Cook]

On Fri, Dec 8, 2017 at 7:29 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Fri, Dec 8, 2017 at 4:29 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>> On 12/07/2017 09:26 PM, Kees Cook wrote:
>>> On Thu, Dec 7, 2017 at 3:33 AM, Eryu Guan <eguan@redhat.com> wrote:
>>>> strscpy() tries to copy sizeof(unsigned long) bytes a time from src
>>>> to dest when possible, and stops the loop when 'max' is less than
>>>> sizeof(unsigned long). But it doesn't check if (src+res) goes beyond
>>>> src buffer and does out-of-bound access to the underlying memory.
>>>>
>>>> KASAN reported global-out-of-bound bug when reading seccomp
>>>> actions_logged file in procfs:
>>>>
>>>>   cat /proc/sys/kernel/seccomp/actions_logged
>>>>
>>>> Because seccomp_names_from_actions_logged() is copying short strings
>>>> (less than sizeof(unsigned long)) to buffer 'names'. e.g.
>>>>
>>>>   ret = strscpy(names, " ", size);
>>>
>>> This is a false positive:
>>> https://marc.info/?l=linux-kernel&m=150768944030805&w=2
>>>
>>> Given that we keep getting these reports (this is the third), I wonder
>>> if can adjust the seccomp code to work around the bug in KASAN...
>>>
>>
>> We should fix this in strscpy() somehow. We just need to decide how to do this.
>> Last time we haven't came to agreement.
>>
>> So, possible solutions are:
>>
>> 1) Simply disable word-at-a-time optimization in strscpy(). I seriously doubt
>> that this optimization have noticeable performance impact in real workloads.
>>
>> 2) Apply patch https://lkml.kernel.org/r/20170718171523.32208-1-aryabinin@virtuozzo.com
>> It's a simple, minimally intrusive way to fix the bug.
>> However, the patch changes semantics/implementation of the strscpy() which is bad idea.
>> It basically means that we use one implementation of the strscpy() but test with KASAN another implementation.
>> For that reason patch wasn't liked by Linus.
>>
>> 3) Introduce read_once_partial_check() function and use it in strscpy().
>> read_once_partial_check() supposed to read one word, but KASAN will check only the first byte of the access.
>> Dmitry didn't like this. I'm also not fan of this solution, because we may not notice some real bugs, e.g.:
>>
>>         char dst[8];
>>         char *src;
>>
>>         src = kmalloc(5, GFP_KERNEL);
>>         memset(src, 0xff, 5);
>>         strscpy(dst, src, 8);
>>
>> stscpy() will copy from 6 up to 8 bytes (exact size depends on what is stored in src[5-7])
>> from non-null terminated src. With read_once_partial_check() used in strscpy() KASAN will
>> not report such bug.
>>
>>
>> So, what it's gonna be? Let's finally decide something and deal with the problem.
>> My vote belongs to 1) or 2).
>
>
> My vote is for 1) if we agree that the optimization is not worth it,
> otherwise for 2).

Who would be the best person to measure the difference for 1)?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Andrey Ryabinin]

On 12/08/2017 11:54 PM, Kees Cook wrote:
> On Fri, Dec 8, 2017 at 7:29 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> On Fri, Dec 8, 2017 at 4:29 PM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>>
>>> So, possible solutions are:
>>>
>>> 1) Simply disable word-at-a-time optimization in strscpy(). I seriously doubt
>>> that this optimization have noticeable performance impact in real workloads.
>>>
>>> 2) Apply patch https://lkml.kernel.org/r/20170718171523.32208-1-aryabinin@virtuozzo.com
>>> It's a simple, minimally intrusive way to fix the bug.
>>> However, the patch changes semantics/implementation of the strscpy() which is bad idea.
>>> It basically means that we use one implementation of the strscpy() but test with KASAN another implementation.
>>> For that reason patch wasn't liked by Linus.
>>>
>>> 3) Introduce read_once_partial_check() function and use it in strscpy().
>>> read_once_partial_check() supposed to read one word, but KASAN will check only the first byte of the access.
>>> Dmitry didn't like this. I'm also not fan of this solution, because we may not notice some real bugs, e.g.:
>>>
>>>         char dst[8];
>>>         char *src;
>>>
>>>         src = kmalloc(5, GFP_KERNEL);
>>>         memset(src, 0xff, 5);
>>>         strscpy(dst, src, 8);
>>>
>>> stscpy() will copy from 6 up to 8 bytes (exact size depends on what is stored in src[5-7])
>>> from non-null terminated src. With read_once_partial_check() used in strscpy() KASAN will
>>> not report such bug.
>>>
>>>
>>> So, what it's gonna be? Let's finally decide something and deal with the problem.
>>> My vote belongs to 1) or 2).
>>
>>
>> My vote is for 1) if we agree that the optimization is not worth it,
>> otherwise for 2).
> 
> Who would be the best person to measure the difference for 1)?
> 

I suppose that depends on which one strscpy() caller you'd want to test.
Briefly looking at all current users, it doesn't look like they process huge amounts
of data through strscpy(), thus we shouldn't suffer from a slight
performance degradation of strscpy().

RE: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[David Laight]

From: Andrey Ryabinin
> Sent: 11 December 2017 16:44
...
> I suppose that depends on which one strscpy() caller you'd want to test.
> Briefly looking at all current users, it doesn't look like they process huge amounts
> of data through strscpy(), thus we shouldn't suffer from a slight
> performance degradation of strscpy().

Don't most of the fast string functions use the same kind of
optimisations.
strlen() is very likely to do 64 bit reads and then shifts (etc)
to determine whether any of the bytes are zero.

	David

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Andrey Ryabinin]

On 12/12/2017 01:19 PM, David Laight wrote:
> From: Andrey Ryabinin
>> Sent: 11 December 2017 16:44
> ...
>> I suppose that depends on which one strscpy() caller you'd want to test.
>> Briefly looking at all current users, it doesn't look like they process huge amounts
>> of data through strscpy(), thus we shouldn't suffer from a slight
>> performance degradation of strscpy().
> 
> Don't most of the fast string functions use the same kind of
> optimisations.
> strlen() is very likely to do 64 bit reads and then shifts (etc)
> to determine whether any of the bytes are zero.
> 

See for yourself, strscpy() is the only sting function doing this.

> 	David
> 

Re: [PATCH] lib/string: avoid reading beyond src buffer in strscpy

[Linus Torvalds]

On Tue, Dec 12, 2017 at 8:06 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
> See for yourself, strscpy() is the only sting function doing this.

No, strnlen_user() definitely does too.

It's just that KASAN doesn't track user pointers.

And the important strlen() in the kernel is the pathname hashing code,
which *definitely* accesses outside the source, but since it can
actually traverse to another page we have that one annotated too (with
load_unaligned_zeropad()).

So no, strscpy() isn't the only one doing it, it is just the only one
that KASAN catches.

            Linus