On Wed, 2017-02-08 at 13:03 +0100, Arnd Bergmann wrote:
- Moving nla_put_{u8,u16,u32} out of line is probably uncontroversial
and
it helps enough with br_netlink.c, but nl820211 is worse and needs
some
additional fiddling.
Why would that not be sufficient by itself for nl80211?
Btw, what's causing this to start with? Can't the compiler reuse the
stack places?
johannes
On Wed, Feb 8, 2017 at 1:24 PM, Johannes Berg [off-list ref] wrote:
On Wed, 2017-02-08 at 13:03 +0100, Arnd Bergmann wrote:
quoted
- Moving nla_put_{u8,u16,u32} out of line is probably uncontroversial
and
it helps enough with br_netlink.c, but nl820211 is worse and needs
some
additional fiddling.
Why would that not be sufficient by itself for nl80211?
Oddly enough, it seems that it is now. I don't know what I was testing earlier,
but now I don't see any difference between this simple change, and the
modifications
I did on nl820211.c. I started out trying to fix this in nl820211.c
originally and then
later tried the extern nla_put_*(), but didn't think it helped all
that much then.
I'll just submit the simple patch then. ;-)
a) current mainline, arm64 allmodconfig+KASAN,
CONFIG_FRAME_WARN=1024
../net/wireless/nl80211.c: In function 'nl80211_get_mesh_config':
../net/wireless/nl80211.c:5689:1: warning: the frame size of 2336
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_send_iface':
../net/wireless/nl80211.c:2591:1: warning: the frame size of 1120
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_add_commands_unsplit.isra.2':
../net/wireless/nl80211.c:1410:1: warning: the frame size of 2272
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_set_wiphy':
../net/wireless/nl80211.c:2491:1: warning: the frame size of 1376
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_send_wiphy':
../net/wireless/nl80211.c:1895:1: warning: the frame size of 4288
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_send_station.isra.44':
../net/wireless/nl80211.c:4389:1: warning: the frame size of 2240
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_dump_station':
../net/wireless/nl80211.c:4441:1: warning: the frame size of 1456
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_get_station':
../net/wireless/nl80211.c:4478:1: warning: the frame size of 1232
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'cfg80211_del_sta_sinfo':
../net/wireless/nl80211.c:13611:1: warning: the frame size of 1072
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_send_bss.isra.43.constprop':
../net/wireless/nl80211.c:7588:1: warning: the frame size of 1296
bytes is larger than 1024 bytes
b) with patch to move nla_put_* functions out of line:
../net/wireless/nl80211.c: In function 'nl80211_set_wiphy':
../net/wireless/nl80211.c:2491:1: warning: the frame size of 1376
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_dump_station':
../net/wireless/nl80211.c:4441:1: warning: the frame size of 1456
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_get_station':
../net/wireless/nl80211.c:4478:1: warning: the frame size of 1232
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'cfg80211_del_sta_sinfo':
../net/wireless/nl80211.c:13611:1: warning: the frame size of 1072
bytes is larger than 1024 bytes
../net/wireless/nl80211.c: In function 'nl80211_dump_survey':
../net/wireless/nl80211.c:7753:1: warning: the frame size of 1136
bytes is larger than 1024 bytes
c) without --param asan-stack=1, checking just the functions above
against 100 bytes
../net/wireless/nl80211.c: In function 'nl80211_set_wiphy':
../net/wireless/nl80211.c:2491:1: warning: the frame size of 144 bytes
is larger than 100 bytes [-Wframe-larger-than=]
../net/wireless/nl80211.c: In function 'nl80211_send_wiphy':
../net/wireless/nl80211.c:1895:1: warning: the frame size of 224 bytes
is larger than 100 bytes [-Wframe-larger-than=]
../net/wireless/nl80211.c: In function 'nl80211_send_station.isra.44':
../net/wireless/nl80211.c:4389:1: warning: the frame size of 144 bytes
is larger than 100 bytes [-Wframe-larger-than=]
../net/wireless/nl80211.c: In function 'nl80211_dump_station':
../net/wireless/nl80211.c:4441:1: warning: the frame size of 912 bytes
is larger than 100 bytes [-Wframe-larger-than=]
../net/wireless/nl80211.c: In function 'nl80211_get_station':
../net/wireless/nl80211.c:4478:1: warning: the frame size of 864 bytes
is larger than 100 bytes [-Wframe-larger-than=]
../net/wireless/nl80211.c: In function 'cfg80211_del_sta_sinfo':
../net/wireless/nl80211.c:13611:1: warning: the frame size of 864
bytes is larger than 100 bytes [-Wframe-larger-than=]
../net/wireless/nl80211.c: In function 'nl80211_dump_survey':
../net/wireless/nl80211.c:7753:1: warning: the frame size of 112 bytes
is larger than 100 bytes [-Wframe-larger-than=]
Btw, what's causing this to start with? Can't the compiler reuse the
stack places?
I have no idea. It's trying to find out of bounds accesses for
objects on the stack, so maybe it gives each variable a separate
stack location in order to see which one caused problems?
Arnd
2017-02-08 16:10 GMT+03:00 Arnd Bergmann [off-list ref]:
On Wed, Feb 8, 2017 at 1:24 PM, Johannes Berg [off-list ref] wrote:
quoted
Btw, what's causing this to start with? Can't the compiler reuse the
stack places?
I have no idea. It's trying to find out of bounds accesses for
objects on the stack, so maybe it gives each variable a separate
stack location in order to see which one caused problems?
If compiler cannot prove that access to the local variable is valid it
will add redzones around that variable
to be able to detect out of bounds accesses.
For example:
static inline int nla_put_u8(struct sk_buff *skb, int attrtype, u8 value)
{
return nla_put(skb, attrtype, sizeof(u8), &value);
}
compiler will surround 'value' with redzones to catch potential oob
access in nla_put().
Another way to fix this, would be something like this:
#ifdef CONFIG_KASAN
/* don't bloat stack */
#define __noinline_for_kasan __noinline __maybe_unused
#else
#define __noinline_for_kasan inline
#endif
static __noinline_for_kasan int nla_put_u8(struct sk_buff *skb, int
attrtype, u8 value)
{
return nla_put(skb, attrtype, sizeof(u8), &value);
}
From: Johannes Berg
Sent: 08 February 2017 12:24
...
Btw, what's causing this to start with? Can't the compiler reuse the
stack places?
Only if it realises they've gone out of scope - which probably
doesn't happen when the functions are inlined.
The address of the parameter can be saved by the calling function
and used in a later call.
Something like this is valid:
int foo(int *p, int v)
{
static int *sv;
int old = -1;
if (sv) {old = *sv; *sv = v;}
sv = v;
return old;
}
void bar(...) {
int a, b;
...
foo(&a, 0);
...
foo(&b, 1);
...
foo(NULL, 2);
...
If the compiler starts sharing stack it all goes wrong.
David