Re: KASAN+netlink, was: [PATCH] [net-next?] hns: avoid stack overflow with CONFIG_KASAN

Re: KASAN+netlink, was: [PATCH] [net-next?] hns: avoid stack overflow with CONFIG_KASAN

[Johannes Berg]

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

Re: KASAN+netlink, was: [PATCH] [net-next?] hns: avoid stack overflow with CONFIG_KASAN

[Arnd Bergmann]

On Wed, Feb 8, 2017 at 1:24 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> 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?

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

Re: KASAN+netlink, was: [PATCH] [net-next?] hns: avoid stack overflow with CONFIG_KASAN

[Andrey Ryabinin]

2017-02-08 16:10 GMT+03:00 Arnd Bergmann <arnd@arndb.de>:
> On Wed, Feb 8, 2017 at 1:24 PM, Johannes Berg <johannes@sipsolutions.net> wrote:

>
>> 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);
}

RE: KASAN+netlink, was: [PATCH] [net-next?] hns: avoid stack overflow with CONFIG_KASAN

[David Laight]

PiBGcm9tOiBKb2hhbm5lcyBCZXJnDQo+IFNlbnQ6IDA4IEZlYnJ1YXJ5IDIwMTcgMTI6MjQNCi4u
Lg0KPiBCdHcsIHdoYXQncyBjYXVzaW5nIHRoaXMgdG8gc3RhcnQgd2l0aD8gQ2FuJ3QgdGhlIGNv
bXBpbGVyIHJldXNlIHRoZQ0KPiBzdGFjayBwbGFjZXM/DQoNCk9ubHkgaWYgaXQgcmVhbGlzZXMg
dGhleSd2ZSBnb25lIG91dCBvZiBzY29wZSAtIHdoaWNoIHByb2JhYmx5DQpkb2Vzbid0IGhhcHBl
biB3aGVuIHRoZSBmdW5jdGlvbnMgYXJlIGlubGluZWQuDQpUaGUgYWRkcmVzcyBvZiB0aGUgcGFy
YW1ldGVyIGNhbiBiZSBzYXZlZCBieSB0aGUgY2FsbGluZyBmdW5jdGlvbg0KYW5kIHVzZWQgaW4g
YSBsYXRlciBjYWxsLg0KDQpTb21ldGhpbmcgbGlrZSB0aGlzIGlzIHZhbGlkOg0KDQppbnQgZm9v
KGludCAqcCwgaW50IHYpDQp7DQoJc3RhdGljIGludCAqc3Y7DQoJaW50IG9sZCA9IC0xOw0KCWlm
IChzdikge29sZCA9ICpzdjsgKnN2ID0gdjt9DQoJc3YgPSB2Ow0KCXJldHVybiBvbGQ7DQp9DQoN
CnZvaWQgYmFyKC4uLikgew0KCWludCBhLCBiOw0KCS4uLg0KCWZvbygmYSwgMCk7DQoJLi4uDQoJ
Zm9vKCZiLCAxKTsNCgkuLi4NCglmb28oTlVMTCwgMik7DQoJLi4uDQoNCklmIHRoZSBjb21waWxl
ciBzdGFydHMgc2hhcmluZyBzdGFjayBpdCBhbGwgZ29lcyB3cm9uZy4NCg0KCURhdmlkDQoNCg0K