From: Andrey Ryabinin <aryabinin@virtuozzo.com> To: Arnd Bergmann <arnd@arndb.de>, David Laight <David.Laight@aculab.com> Cc: "Mauro Carvalho Chehab" <mchehab@kernel.org>, "Jiri Pirko" <jiri@resnulli.us>, "Arend van Spriel" <arend.vanspriel@broadcom.com>, "Kalle Valo" <kvalo@codeaurora.org>, "David S. Miller" <davem@davemloft.net>, "Alexander Potapenko" <glider@google.com>, "Dmitry Vyukov" <dvyukov@google.com>, "Masahiro Yamada" <yamada.masahiro@socionext.com>, "Michal Marek" <mmarek@suse.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Kees Cook" <keescook@chromium.org>, "Geert Uytterhoeven" <geert@linux-m68k.org>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>, "brcm80211-dev-list.pdl@broadcom.com" <brcm80211-dev-list.pdl@broadcom.com>, "brcm80211-dev-list@cypress.com" <brcm80211-dev-list@cypress.com>, "kasan-dev@googlegroups.com" <kasan-dev@googlegroups.com>, "linux-kbuild@vger.kernel.org" <linux-kbuild@vger.kernel.org>, "Jakub Jelinek" <jakub@gcc.gnu.org>, "Martin Liška" <marxin@gcc.gnu.org>, "stable@vger.kernel.org" <stable@vger.kernel.org> Subject: Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Date: Tue, 26 Sep 2017 19:49:48 +0300 [thread overview] Message-ID: <e7e6418e-4340-5057-aa17-800082aca5fb@virtuozzo.com> (raw) In-Reply-To: <CAK8P3a37Ts5q7BvA2JWse87huyAp+=e18CUXEt8731RrBnB+Ow@mail.gmail.com> 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: >> On Mon, Sep 25, 2017 at 7:41 AM, David Laight <David.Laight@aculab.com> wrote: >>> From: Arnd Bergmann >>>> Sent: 22 September 2017 22:29 >>> ... >>>> It seems that this is triggered in part by using strlcpy(), which the >>>> compiler doesn't recognize as copying at most 'len' bytes, since strlcpy >>>> is not part of the C standard. >>> >>> Neither is strncpy(). >>> >>> It'll almost certainly be a marker in a header file somewhere, >>> so it should be possibly to teach it about other functions. >> >> I'm currently travelling and haven't investigated in detail, but from >> taking a closer look here, I found that the hardened 'strlcpy()' >> in include/linux/string.h triggers it. There is also a hardened >> (much shorted) 'strncpy()' that doesn't trigger it in the same file, >> and having only the extern declaration of strncpy also doesn't. > > And a little more experimenting leads to this simple patch that fixes > the problem: > > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -254,7 +254,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const > char *q, size_t size) > size_t q_size = __builtin_object_size(q, 0); > if (p_size == (size_t)-1 && q_size == (size_t)-1) > return __real_strlcpy(p, q, size); > - ret = strlen(q); > + 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. > 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: --- 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; } > 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. - 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.
WARNING: multiple messages have this Message-ID (diff)
From: Andrey Ryabinin <aryabinin@virtuozzo.com> To: Arnd Bergmann <arnd@arndb.de>, David Laight <David.Laight@aculab.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>, Jiri Pirko <jiri@resnulli.us>, Arend van Spriel <arend.vanspriel@broadcom.com>, Kalle Valo <kvalo@codeaurora.org>, "David S. Miller" <davem@davemloft.net>, Alexander Potapenko <glider@google.com>, Dmitry Vyukov <dvyukov@google.com>, Masahiro Yamada <yamada.masahiro@socionext.com>, Michal Marek <mmarek@suse.com>, Andrew Morton <akpm@linux-foundation.org>, Kees Cook <keescook@chromium.org>, Geert Uytterhoeven <geert@linux-m68k.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org> Subject: Re: [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Date: Tue, 26 Sep 2017 19:49:48 +0300 [thread overview] Message-ID: <e7e6418e-4340-5057-aa17-800082aca5fb@virtuozzo.com> (raw) In-Reply-To: <CAK8P3a37Ts5q7BvA2JWse87huyAp+=e18CUXEt8731RrBnB+Ow@mail.gmail.com> 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: >> On Mon, Sep 25, 2017 at 7:41 AM, David Laight <David.Laight@aculab.com> wrote: >>> From: Arnd Bergmann >>>> Sent: 22 September 2017 22:29 >>> ... >>>> It seems that this is triggered in part by using strlcpy(), which the >>>> compiler doesn't recognize as copying at most 'len' bytes, since strlcpy >>>> is not part of the C standard. >>> >>> Neither is strncpy(). >>> >>> It'll almost certainly be a marker in a header file somewhere, >>> so it should be possibly to teach it about other functions. >> >> I'm currently travelling and haven't investigated in detail, but from >> taking a closer look here, I found that the hardened 'strlcpy()' >> in include/linux/string.h triggers it. There is also a hardened >> (much shorted) 'strncpy()' that doesn't trigger it in the same file, >> and having only the extern declaration of strncpy also doesn't. > > And a little more experimenting leads to this simple patch that fixes > the problem: > > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -254,7 +254,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const > char *q, size_t size) > size_t q_size = __builtin_object_size(q, 0); > if (p_size == (size_t)-1 && q_size == (size_t)-1) > return __real_strlcpy(p, q, size); > - ret = strlen(q); > + 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. > 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: --- 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; } > 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. - 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.
next prev parent reply other threads:[~2017-09-26 16:47 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-09-22 21:29 [PATCH v4 0/9] bring back stack frame warning with KASAN Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-22 21:29 ` [PATCH v4 1/9] brcmsmac: make some local variables 'static const' to reduce stack size Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-25 4:33 ` Kalle Valo 2017-09-25 4:33 ` Kalle Valo 2017-10-02 13:53 ` [v4, " Kalle Valo 2017-10-02 13:53 ` Kalle Valo 2017-10-02 13:53 ` Kalle Valo 2017-09-22 21:29 ` [PATCH v4 2/9] brcmsmac: split up wlc_phy_workarounds_nphy Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-10-02 13:55 ` [v4,2/9] " Kalle Valo 2017-10-02 13:55 ` Kalle Valo 2017-10-02 13:55 ` Kalle Valo 2017-10-27 7:51 ` Kalle Valo 2017-10-27 7:51 ` Kalle Valo 2017-10-27 7:51 ` Kalle Valo 2017-09-22 21:29 ` [PATCH v4 3/9] brcmsmac: reindent split functions Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-22 21:29 ` [PATCH v4 4/9] em28xx: fix em28xx_dvb_init for KASAN Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-25 14:41 ` David Laight 2017-09-25 14:41 ` David Laight 2017-09-26 6:32 ` Arnd Bergmann 2017-09-26 6:32 ` Arnd Bergmann 2017-09-26 6:47 ` Arnd Bergmann 2017-09-26 6:47 ` Arnd Bergmann 2017-09-26 16:49 ` Andrey Ryabinin [this message] 2017-09-26 16:49 ` Andrey Ryabinin 2017-09-27 13:26 ` Arnd Bergmann 2017-09-27 13:26 ` Arnd Bergmann 2017-09-28 13:09 ` Andrey Ryabinin 2017-09-28 13:09 ` Andrey Ryabinin 2017-09-28 14:30 ` Arnd Bergmann 2017-09-28 14:30 ` Arnd Bergmann 2017-10-02 8:33 ` Arnd Bergmann 2017-10-02 8:33 ` Arnd Bergmann 2017-10-02 8:40 ` [PATCH] string.h: work around for increased stack usage Arnd Bergmann 2017-10-02 9:02 ` Arnd Bergmann 2017-10-02 14:07 ` Andrey Ryabinin 2017-10-03 18:10 ` kbuild test robot 2017-10-03 18:10 ` kbuild test robot 2017-09-22 21:29 ` [PATCH v4 5/9] r820t: fix r820t_write_reg for KASAN Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-22 21:29 ` [PATCH v4 6/9] dvb-frontends: fix i2c access helpers " Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-22 21:29 ` [PATCH v4 7/9] rocker: fix rocker_tlv_put_* functions " Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-26 3:19 ` David Miller 2017-09-22 21:29 ` [PATCH v4 8/9] netlink: fix nla_put_{u8,u16,u32} " Arnd Bergmann 2017-09-22 21:29 ` Arnd Bergmann 2017-09-26 3:19 ` David Miller 2017-09-22 21:29 ` [PATCH v4 9/9] kasan: rework Kconfig settings Arnd Bergmann 2017-09-26 19:36 ` Andrey Ryabinin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e7e6418e-4340-5057-aa17-800082aca5fb@virtuozzo.com \ --to=aryabinin@virtuozzo.com \ --cc=David.Laight@aculab.com \ --cc=akpm@linux-foundation.org \ --cc=arend.vanspriel@broadcom.com \ --cc=arnd@arndb.de \ --cc=brcm80211-dev-list.pdl@broadcom.com \ --cc=brcm80211-dev-list@cypress.com \ --cc=davem@davemloft.net \ --cc=dvyukov@google.com \ --cc=geert@linux-m68k.org \ --cc=glider@google.com \ --cc=gregkh@linuxfoundation.org \ --cc=jakub@gcc.gnu.org \ --cc=jiri@resnulli.us \ --cc=kasan-dev@googlegroups.com \ --cc=keescook@chromium.org \ --cc=kvalo@codeaurora.org \ --cc=linux-kbuild@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-wireless@vger.kernel.org \ --cc=marxin@gcc.gnu.org \ --cc=mchehab@kernel.org \ --cc=mmarek@suse.com \ --cc=netdev@vger.kernel.org \ --cc=stable@vger.kernel.org \ --cc=yamada.masahiro@socionext.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.