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: <1467293149.11238.12.camel@edumazet-glaptop3.roam.corp.google.com>
Date:	Thu, 30 Jun 2016 15:25:49 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Dmitry Vyukov <dvyukov@...gle.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, 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

Same trick was used in
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=885a136c52a8871175477baf3903e1c38751b35a