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 for Android: free password hash cracker in your pocket

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

Date: Thu, 22 Jun 2023 21:00:43 +0200
From: Eric Dumazet <edumazet@...gle.com>
To: Jay Vosburgh <jay.vosburgh@...onical.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

On Thu, Jun 22, 2023 at 8:55 PM Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
>
> 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.
>

Ah right, I will send another patch to remove it then.

I think it makes sense to keep the first patch small for backports.

History of relevant patches :

from 5.17

429e3d123d9a50cc9882402e40e0ac912d88cfcf bonding: Fix extraction of
ports from the packet headers

from 5.15

a815bde56b15ce626caaacc952ab12501671e45d net, bonding: Refactor
bond_xmit_hash for use with xdp_buff