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
| ||
|
Message-ID: <CACRpkdbo=Oem4PCOtSV6iWJoojRetTgZhx7J91uecTa-DQA8iQ@mail.gmail.com> Date: Wed, 20 Dec 2023 01:43:55 +0100 From: Linus Walleij <linus.walleij@...aro.org> To: Vladimir Oltean <olteanv@...il.com> Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>, Romain Gantois <romain.gantois@...tlin.com>, Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Maxime Coquelin <mcoquelin.stm32@...il.com>, Miquel Raynal <miquel.raynal@...tlin.com>, Sylvain Girard <sylvain.girard@...com>, Pascal EBERHARD <pascal.eberhard@...com>, Richard Tresidder <rtresidd@...ctromag.com.au>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, linux-arm-kernel@...ts.infradead.org, stable@...r.kernel.org Subject: Re: [PATCH net 1/1] net: stmmac: Prevent DSA tags from breaking COE On Tue, Dec 19, 2023 at 11:46 PM Vladimir Oltean <olteanv@...il.com> wrote: > On Tue, Dec 19, 2023 at 05:29:32PM +0100, Maxime Chevallier wrote: > > > I can certainly add a helper such as skb_eth_raw_ethertype() > > > to <linux/if_ether.h> that will inspect the actual ethertype in > > > skb->data. > > > > > > It's the most straight-forward approach. > > > > Agreed :) > > If you rewrite that patch to use skb_vlan_eth_hdr() to get a struct > vlan_ethhdr pointer through which h_vlan_proto and h_vlan_encapsulated_proto > are accessible, I don't see much value in writing that helper. It is > going to beg the question how generic should it be - should it also > treat ETH_P_8021AD, should it treat nested VLANs? I guess I should just post the patches inline. (It came from both Erics and Maximes suggestion really.) Actually I wrote two helpers, one to get the ethertype from the ethernet frame which is pretty straight-forward. include/linux/if_ether.h +/* This determines the ethertype incoded into the skb data without + * relying on skb->protocol which is not always identical. + */ +static inline u16 skb_eth_raw_ethertype(const struct sk_buff *skb) +{ + struct ethhdr *hdr; + + /* If we can't extract a header, return invalid type */ + if (!skb_pointer_if_linear(skb, 0, ETH_HLEN)) + return 0x0000U; + + hdr = skb_eth_hdr(skb); + + return ntohs(hdr->h_proto); +} Then for *this* driver I need to check for the ethertype ETH_P_8021Q what is inside it, one level down, and that is a separate helper. And I named it skb_vlan_raw_inner_ethertype() It will retrieve the inner type no matter include/linux/if_vlan.h +/* This determines the inner ethertype incoded into the skb data without + * relying on skb->protocol which is not always identical. + */ +static inline u16 skb_vlan_raw_inner_ethertype(const struct sk_buff *skb) +{ + struct vlan_ethhdr *vhdr; + + if (!skb_pointer_if_linear(skb, 0, VLAN_ETH_HLEN)) + return 0x0000U; + + vhdr = vlan_eth_hdr(skb); + return ntohs(vhdr->h_vlan_encapsulated_proto); +} (We can bikeshed the name of the function. *_inner_protocol maybe.) It does not handle nested VLANs and I don't see why it should since the immediate siblings in if_vlan.h does not, i.e. vlan_eth_hdr(), skb_vlan_eth_hdr(). It's pretty clear these helpers all go just one level down. (We can add a *_descend_*() helper the day someone needs that.) > At the end of the day, you are trying to cover in software the cases for > which the hardware engine can perform TX checksum offloading. That is > going to be hardware specific. Yeps and I am happy to fold these helpers inside of my driver if they are not helpful to anyone else, or if that is the best idea for something intended for a fix, i.e. an -rc kernel. > I guess we should first try to answer the questions "what does > skb->protocol represent?" and "does DSA use it correctly?" before > even thinking about adding yet another fuzzy layer on top it. Fair point! Let's take a step back. The kerneldoc says: * @protocol: Packet protocol from driver That's a bit vague and it was in the first commit in git history :/ But Eric probably knows the right way to use protocol. But we know for sure that VLAN uses this for the outermost protocol ETH_P_8021Q (etc). I wonder how the network stack reacts if we set the skb->protocol to whatever DSA taggers put at the position of the ethertype. For RTL taggers probably this works because they use an elaborate custom ethertype, but e.g. net/dsa/tag_mtk.c will just put in "ethertype" 0x0000, 0x0001 or 0x0002, the two latter which are formally ETH_P_802_3 and ETH_P_AX25 which I think is maybe not so good to put into skb->protocol. Another option is to set it to the ETH_P_DSA ethertype, currently unused in the kernel. Now this kind of thinking makes me insecure because: git grep '\->protocol' net/ There is just sooooo much code inspecting ->protocol in the generic network stack that this seems like inviting disaster. Yours, Linus Walleij
Powered by blists - more mailing lists