netdev - Re: [PATCH net] bonding: prevent out of bound accesses

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+YdOi1-z1pPJaYBybyHZ=MXzHPGjGU+UJWJf-N3pgGzeg@mail.gmail.com>
Date:	Thu, 30 Jun 2016 15:35:51 +0200
From:	Dmitry Vyukov <dvyukov@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>,
	netdev <netdev@...r.kernel.org>,
	Mahesh Bandewar <maheshb@...gle.com>,
	Nikolay Aleksandrov <nikolay@...hat.com>,
	Ding Tianhong <dingtianhong@...wei.com>
Subject: Re: [PATCH net] bonding: prevent out of bound accesses

On Thu, Jun 30, 2016 at 3:25 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Thu, 2016-06-30 at 15:04 +0200, Dmitry Vyukov wrote:
>> On Thu, Jun 30, 2016 at 2:56 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>> > From: Eric Dumazet <edumazet@...gle.com>
>> >
>> > ether_addr_equal_64bits() requires some care about its arguments,
>> > namely that 8 bytes might be read, even if last 2 byte values are not
>> > used.
>> >
>> > KASan detected a violation with null_mac_addr and lacpdu_mcast_addr
>> > in bond_3ad.c
>> >
>> > Fixes: 815117adaf5b ("bonding: use ether_addr_equal_unaligned for bond addr compare")
>> > Fixes: bb54e58929f3 ("bonding: Verify RX LACPDU has proper dest mac-addr")
>> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>> > Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
>> > ---
>> >  drivers/net/bonding/bond_3ad.c |   11 +++++++----
>> >  drivers/net/bonding/bond_alb.c |    3 ---
>> >  include/net/bonding.h          |    7 ++++++-
>> >  3 files changed, 13 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
>> > index ca81f46ea1aa..8ad491ab1d01 100644
>> > --- a/drivers/net/bonding/bond_3ad.c
>> > +++ b/drivers/net/bonding/bond_3ad.c
>> > @@ -101,11 +101,14 @@ enum ad_link_speed_type {
>> >  #define MAC_ADDRESS_EQUAL(A, B)        \
>> >         ether_addr_equal_64bits((const u8 *)A, (const u8 *)B)
>> >
>> > -static struct mac_addr null_mac_addr = { { 0, 0, 0, 0, 0, 0 } };
>> > +static const u8 null_mac_addr[ETH_ALEN] __long_aligned = {
>> > +       0, 0, 0, 0, 0, 0
>> > +};
>>
>> __long_aligned won't prevent KASAN from barking, it's still 6 bytes
>> but we are reading 8.
>> Can we please do "ETH_ALEN + 2" as below?
>
> Really ?
>
> sizeof(null_mac_addr) is 8, how KASan would decide it is 6 bytes ?
>
> nm -v drivers/net/bonding/bonding.ko|grep -3 null_mac_addr
>
> 000000000000084a r __UNIQUE_ID_mode17
> 0000000000000850 r tmpl.46263
> 0000000000000860 r lacpdu_mcast_addr
> 0000000000000868 r null_mac_addr
> 0000000000000870 r mac_v6_allmcast
> 0000000000000878 r mac_bcast
> 0000000000000880 r slave_sysfs_ops


What does nm -S say?
I've tried to compile a sample program with gcc6 and
__attribute__((aligned(8))) does in fact force alignment of start of
the symbol, but size still stays 6. (and asan still barks on my test
program).