netdev - Re: [PATCH net] bonding: do not assume skb mac

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]

Date: Thu, 22 Jun 2023 11:55:44 -0700
From: Jay Vosburgh <jay.vosburgh@...onical.com>
To: Eric Dumazet <edumazet@...gle.com>
cc: "David S . Miller" <davem@...emloft.net>,
    Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
    netdev@...r.kernel.org, eric.dumazet@...il.com,
    syzbot <syzkaller@...glegroups.com>, Jarod Wilson <jarod@...hat.com>,
    Moshe Tal <moshet@...dia.com>, Jussi Maki <joamaki@...il.com>,
    Andy Gospodarek <andy@...yhouse.net>,
    Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net] bonding: do not assume skb mac_header is set

Eric Dumazet <edumazet@...gle.com> wrote:

>On Thu, Jun 22, 2023 at 7:48 PM Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
>>
>> Eric Dumazet <edumazet@...gle.com> wrote:

[...]

>> > drivers/net/bonding/bond_main.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >index edbaa1444f8ecd9bf344a50f6f599d7eaaf4ff3e..091e035c76a6ff29facbaf1c0f26d185dc8ff5e3 100644
>> >--- a/drivers/net/bonding/bond_main.c
>> >+++ b/drivers/net/bonding/bond_main.c
>> >@@ -4197,7 +4197,7 @@ u32 bond_xmit_hash(struct bonding *bond, struct sk_buff *skb)
>> >               return skb->hash;
>> >
>> >       return __bond_xmit_hash(bond, skb, skb->data, skb->protocol,
>> >-                              skb_mac_offset(skb), skb_network_offset(skb),
>> >+                              0, skb_network_offset(skb),
>> >                               skb_headlen(skb));
>> > }
>>
>>         Is the MAC header guaranteed to be at skb->data, then?  If not,
>> then isn't replacing skb_mac_offset() with 0 going to break the hash (as
>> it might or might not be looking at the actual MAC header)?
>>
>
>In ndo_start_xmit(), skb->data points to MAC header by definition.

	Ok.

>>         Also, assuming for the moment that this change is ok, this makes
>> all callers of __bond_xmit_hash() supply zero for the mhoff parameter,
>> and a complete fix should therefore remove the unused parameter and its
>> various references.
>
>Not really: bond_xmit_hash_xdp() calls __bond_xmit_hash() with
>sizeof(struct ethhdr)

	I don't think so:

static u32 __bond_xmit_hash(struct bonding *bond, struct sk_buff *skb, const void *data,
                            __be16 l2_proto, int mhoff, int nhoff, int hlen)
{

	"mhoff", currently supplied as skb_mac_offset(skb) in
bond_xmit_hash(), is the fifth parameter.

static u32 bond_xmit_hash_xdp(struct bonding *bond, struct xdp_buff *xdp)
{
[...]
        return __bond_xmit_hash(bond, NULL, xdp->data, eth->h_proto, 0,
                                sizeof(struct ethhdr), xdp->data_end - xdp->data);
}

	The fifth argument here is 0.

	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com