netdev - Re: [PATCH net 1/4] bonding: fix send_peer

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: <ZEJdfWNwzfjpTXom@Laptop-X1>
Date:   Fri, 21 Apr 2023 17:55:09 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     Jakub Kicinski <kuba@...nel.org>,
        kernel test robot <lkp@...el.com>, netdev@...r.kernel.org,
        oe-kbuild-all@...ts.linux.dev,
        "David S . Miller" <davem@...emloft.net>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Liang Li <liali@...hat.com>, Vincent Bernat <vincent@...nat.ch>
Subject: Re: [PATCH net 1/4] bonding: fix send_peer_notif overflow

On Thu, Apr 20, 2023 at 10:13:17PM -0700, Jay Vosburgh wrote:
> >It looks define send_peer_notif to u64 is a bit too large, which introduce
> >complex conversion for 32bit arch.
> >
> >For the remainder operation,
> >bond->send_peer_notif % max(1, bond->params.peer_notif_delay). u32 % u32 look OK.
> >
> >But for multiplication operation,
> >bond->send_peer_notif = bond->params.num_peer_notif * max(1, bond->params.peer_notif_delay);
> >It's u8 * u32. How about let's limit the peer_notif_delay to less than max(u32 / u8),
> >then we can just use u32 for send_peer_notif. Is there any realistic meaning
> >to set peer_notif_delay to max(u32)? I don't think so.
> >
> >Jay, what do you think?
> 
> 	I'm fine to limit the peerf_notif_delay range and then use a
> smaller type.
> 
> 	num_peer_notif is already limited to 255; I'm going to suggest a
> limit to the delay of 300 seconds.  That seems like an absurdly long
> time for this; I didn't do any kind of science to come up with that
> number.
> 
> 	As peer_notif_delay is stored in units of miimon intervals, that
> gives a worst case peer_notif_delay value of 300000 if miimon is 1, and
> 255 * 300000 fits easily in a u32 for send_peer_notif.

OK, I just found another overflow. In bond_fill_info(),
or bond_option_miimon_set():

        if (nla_put_u32(skb, IFLA_BOND_PEER_NOTIF_DELAY,
                        bond->params.peer_notif_delay * bond->params.miimon))
                goto nla_put_failure;

Since both peer_notif_delay and miimon are defined as int, there is a
possibility that the fill in number got overflowed. The same with up/down delay.

Even we limit the peer_notif_delay to 300s, which is 30000, there is still has
possibility got overflowed if we set miimon large enough.

This overflow should only has effect on use space shown since it's a
multiplication result. The kernel part works fine. I'm not sure if we should
also limit the miimon, up/down delay values..

Thanks
Hangbin