Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1740724 > unrolled thread

Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Arnd Bergmann <arnd@arndb.de> - 2017-09-27 15:30 +0200
    Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Arnd Bergmann <arnd@arndb.de> - 2017-09-28 16:40 +0200
      Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Arnd Bergmann <arnd@arndb.de> - 2017-10-02 10:40 +0200
        [PATCH] string.h: work around for increased stack usage Arnd Bergmann <arnd@arndb.de> - 2017-10-02 10:50 +0200
          Re: [PATCH] string.h: work around for increased stack usage Arnd Bergmann <arnd@arndb.de> - 2017-10-02 11:10 +0200

#1740724 — Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN

On Tue, Sep 26, 2017 at 9:49 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
>
>
> On 09/26/2017 09:47 AM, Arnd Bergmann wrote:
>> On Mon, Sep 25, 2017 at 11:32 PM, Arnd Bergmann <arnd@arndb.de> wrote:

>> +       ret = __builtin_strlen(q);
>
>
> I think this is not correct. Fortified strlen called here on purpose. If sizeof q is known at compile time
> and 'q' contains not-null fortified strlen() will panic.

Ok, got it.

>>         if (size) {
>>                 size_t len = (ret >= size) ? size - 1 : ret;
>>                 if (__builtin_constant_p(len) && len >= p_size)
>>
>> The problem is apparently that the fortified strlcpy calls the fortified strlen,
>> which in turn calls strnlen and that ends up calling the extern '__real_strnlen'
>> that gcc cannot reduce to a constant expression for a constant input.
>
>
> Per my observation, it's the code like this:
>         if ()
>                 fortify_panic(__func__);
>
>
> somehow prevent gcc to merge several "struct i2c_board_info info;" into one stack slot.
> With the hack bellow, stack usage reduced to ~1,6K:

1.6k is also what I see with my patch, or any other approach I tried
that changes
string.h. With the split up em28xx_dvb_init() function (and without
changes to string.h),
I got down to a few hundred bytes for the largest handler.

> ---
>  include/linux/string.h | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 54d21783e18d..9a96ff3ebf94 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -261,8 +261,6 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p)
>         if (p_size == (size_t)-1)
>                 return __builtin_strlen(p);
>         ret = strnlen(p, p_size);
> -       if (p_size <= ret)
> -               fortify_panic(__func__);
>         return ret;
>  }
>
> @@ -271,8 +269,6 @@ __FORTIFY_INLINE __kernel_size_t strnlen(const char *p, __kernel_size_t maxlen)
>  {
>         size_t p_size = __builtin_object_size(p, 0);
>         __kernel_size_t ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
> -       if (p_size <= ret && maxlen != ret)
> -               fortify_panic(__func__);
>         return ret;

I've reduced it further to this change:

--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -227,7 +227,7 @@ static inline const char *kbasename(const char *path)
 #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
 #define __RENAME(x) __asm__(#x)

-void fortify_panic(const char *name) __noreturn __cold;
+void fortify_panic(const char *name) __cold;
 void __read_overflow(void) __compiletime_error("detected read beyond
size of object passed as 1st parameter");
 void __read_overflow2(void) __compiletime_error("detected read beyond
size of object passed as 2nd parameter");
 void __read_overflow3(void) __compiletime_error("detected read beyond
size of object passed as 3rd parameter");

I don't immediately see why the __noreturn changes the behavior here, any idea?

>> Not sure if that change is the best fix, but it seems to address the problem in
>> this driver and probably leads to better code in other places as well.
>>
>
> Probably it would be better to solve this on the strlcpy side, but I haven't found the way to do this right.
> Alternative solutions:
>
>  - use memcpy() instead of strlcpy(). All source strings are smaller than I2C_NAME_SIZE, so we could
>    do something like this - memcpy(info.type, "si2168", sizeof("si2168"));
>    Also this should be faster.

This would be very similar to the patch I posted at the start of this
thread to use strncpy(), right?
I was hoping that changing strlcpy() here could also improve other
users that might run into
the same situation, but stay below the 2048-byte stack frame limit.

>  - Move code under different "case:" in the switch(dev->model) to the separate function should help as well.
>    But it might be harder to backport into stables.

Agreed, I posted this in earlier versions of the patch series, see
https://patchwork.kernel.org/patch/9601025/

The new patch was a result of me trying to come up with a less
invasive version to
make it easier to backport, since I would like to backport the last
patch in the series
that depends on all the earlier ones.

         Arnd

[toc] | [next] | [standalone]

#1741600

On Thu, Sep 28, 2017 at 6:09 AM, Andrey Ryabinin
<aryabinin@virtuozzo.com> wrote:
> On 09/27/2017 04:26 PM, Arnd Bergmann wrote:
>> On Tue, Sep 26, 2017 at 9:49 AM, Andrey Ryabinin
>> <aryabinin@virtuozzo.com> wrote:

>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -227,7 +227,7 @@ static inline const char *kbasename(const char *path)
>>  #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
>>  #define __RENAME(x) __asm__(#x)
>>
>> -void fortify_panic(const char *name) __noreturn __cold;
>> +void fortify_panic(const char *name) __cold;
>>  void __read_overflow(void) __compiletime_error("detected read beyond
>> size of object passed as 1st parameter");
>>  void __read_overflow2(void) __compiletime_error("detected read beyond
>> size of object passed as 2nd parameter");
>>  void __read_overflow3(void) __compiletime_error("detected read beyond
>> size of object passed as 3rd parameter");
>>
>> I don't immediately see why the __noreturn changes the behavior here, any idea?
>>
>
>
> At first I thought that this somehow might be related to __asan_handle_no_return(). GCC calls it
> before noreturn function. So I made patch to remove generation of these calls (we don't need them in the kernel anyway)
> but it didn't help. It must be something else than.

I made a reduced test case yesterday (see http://paste.ubuntu.com/25628030/),
and it shows the same behavior with and without the sanitizer, it uses 128
bytes without the noreturn attribute and 480 bytes when its added, the sanitizer
adds a factor of 1.5x on top. It's possible that I did something wrong while
reducing, since the original driver file uses very little stack (a few hundred
bytes) without -fsanitize=kernel-address, but finding out what happens in
the reduced case may still help understand the other one.

        Arnd

[toc] | [prev] | [next] | [standalone]

#1743019

On Thu, Sep 28, 2017 at 4:30 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Sep 28, 2017 at 6:09 AM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>> On 09/27/2017 04:26 PM, Arnd Bergmann wrote:
>>> On Tue, Sep 26, 2017 at 9:49 AM, Andrey Ryabinin
>>> <aryabinin@virtuozzo.com> wrote:
>
>>> --- a/include/linux/string.h
>>> +++ b/include/linux/string.h
>>> @@ -227,7 +227,7 @@ static inline const char *kbasename(const char *path)
>>>  #define __FORTIFY_INLINE extern __always_inline __attribute__((gnu_inline))
>>>  #define __RENAME(x) __asm__(#x)
>>>
>>> -void fortify_panic(const char *name) __noreturn __cold;
>>> +void fortify_panic(const char *name) __cold;
>>>  void __read_overflow(void) __compiletime_error("detected read beyond
>>> size of object passed as 1st parameter");
>>>  void __read_overflow2(void) __compiletime_error("detected read beyond
>>> size of object passed as 2nd parameter");
>>>  void __read_overflow3(void) __compiletime_error("detected read beyond
>>> size of object passed as 3rd parameter");
>>>
>>> I don't immediately see why the __noreturn changes the behavior here, any idea?
>>>
>>
>>
>> At first I thought that this somehow might be related to __asan_handle_no_return(). GCC calls it
>> before noreturn function. So I made patch to remove generation of these calls (we don't need them in the kernel anyway)
>> but it didn't help. It must be something else than.
>
> I made a reduced test case yesterday (see http://paste.ubuntu.com/25628030/),
> and it shows the same behavior with and without the sanitizer, it uses 128
> bytes without the noreturn attribute and 480 bytes when its added, the sanitizer
> adds a factor of 1.5x on top. It's possible that I did something wrong while
> reducing, since the original driver file uses very little stack (a few hundred
> bytes) without -fsanitize=kernel-address, but finding out what happens in
> the reduced case may still help understand the other one.

This is now GCC PR82365, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365

I've come up with a workaround, but I'm not sure if that is any better than the
alternatives, will send the patch as a follow-up in a bit.

     Arnd

[toc] | [prev] | [next] | [standalone]

#1743023 — [PATCH] string.h: work around for increased stack usage

The hardened strlen() function causes rather large stack usage
in at least one file in the kernel when CONFIG_KASAN is enabled:

drivers/media/usb/em28xx/em28xx-dvb.c: In function 'em28xx_dvb_init':
drivers/media/usb/em28xx/em28xx-dvb.c:2062:1: error: the frame size of 3256 bytes is larger than 204 bytes [-Werror=frame-larger-than=]

Analyzing this problem led to the discovery that gcc fails to
merge the stack slots for the i2c_board_info[] structures after
we strlcpy() into them, due to the 'noreturn' attribute on the
source string length check.

The compiler behavior should get fixed in gcc-8, but for users
of existing gcc versions, we can work around it using an empty
inline assembly statement before the call to fortify_panic().

The workaround is unfortunately very ugly, and I tried my best
to limit it being applied to affected versions of gcc when
KASAN is used. Alternative suggestions welcome.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82365
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 include/linux/string.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index c7a1132cdc93..1bf5ecdf8e01 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -228,6 +228,16 @@ static inline const char *kbasename(const char *path)
 #define __RENAME(x) __asm__(#x)
 
 void fortify_panic(const char *name) __noreturn __cold;
+
+/* work around GCC PR82365 */
+#if defined(CONFIG_KASAN) && !defined(__clang__) && GCC_VERSION <= 80000
+#define fortify_panic(x) \
+	do { \
+		asm volatile(""); \
+		fortify_panic(x); \
+	} while (0)
+#endif
+
 void __read_overflow(void) __compiletime_error("detected read beyond size of object passed as 1st parameter");
 void __read_overflow2(void) __compiletime_error("detected read beyond size of object passed as 2nd parameter");
 void __read_overflow3(void) __compiletime_error("detected read beyond size of object passed as 3rd parameter");
-- 
2.9.0

[toc] | [prev] | [next] | [standalone]

#1743029 — Re: [PATCH] string.h: work around for increased stack usage

On Mon, Oct 2, 2017 at 10:40 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>
>  void fortify_panic(const char *name) __noreturn __cold;
> +
> +/* work around GCC PR82365 */
> +#if defined(CONFIG_KASAN) && !defined(__clang__) && GCC_VERSION <= 80000
> +#define fortify_panic(x) \
> +       do { \
> +               asm volatile(""); \
> +               fortify_panic(x); \
> +       } while (0)
> +#endif

This broke the build for the fortify_panic() definition in lib/string.c which
clashes with the macro. I've fixed it locally by renaming it to __fortify_panic,
but won't post the fixed version until I get some feedback on the basic
approach.

      Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web