netdev - Re: [PATCH net-next] net: ptp: get rid of IPV4_HLEN() and OFF

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: <2135183.8zsl8bQRyA@n95hx1g2>
Date:   Fri, 16 Oct 2020 09:04:38 +0200
From:   Christian Eggers <ceggers@...i.de>
To:     Florian Fainelli <f.fainelli@...il.com>
CC:     Richard Cochran <richardcochran@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Krzysztof Halasa <khalasa@...p.pl>,
        Vishal Kulkarni <vishal@...lsio.com>, <netdev@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next] net: ptp: get rid of IPV4_HLEN() and OFF_IHL macros

On Thursday, 15 October 2020, 18:56:41 CEST, Florian Fainelli wrote:
> Having recently helped someone with a bug that involved using
> IPV4_HLEN() instead of ip_hdr() in a driver's transmit path (so with the
> transport header correctly set), it would probably help if we could make
> IPV4_HLEN()'s semantics clearer with converting it to a static inline
> function and put asserts in there about what the transport and MAC
> header positions should be.
IPV4_HLEN() is only used for PTP. Is there any way to use "normal" 
infrastructure as in the rest of the network stack? It looks like PTP code 
typically has to look into multiple network layers (mac, network, transport) 
at places these layers may not be processed yet (at least in RX direction).

> At the very least, creating a new function, like this maybe in
> include/linux/ip.h might help:
> 
> static inline u8 __ip_hdr_len(const struct sk_buff *skb)
> {
> 	const struct iphdr *ip_hdr = (const struct iphdr *)(skb->data);
> 
> 	return ip_hdr->ihl << 2;
> }
Is there any reason using skb->data instead of skb_network_header()? Debugging 
through my DSA driver showed that ...

- for TX (called by dsa_slave_xmit), skb->data is set to the MAC header
(skb->head+0x02), whilst skb->network_header is correctly set to 0x10 
(skb->mac_header+14).
- for TX, skb->transport_header is 0xffff, so udp_hdr() cannot be used

- for RX (called by dsa_switch_rcv), skb->data is set to skb->head+0x50, which 
is identical to skb->network_header
- for RX, skb->transport_header ist set equal to skb->network_header, so 
udp_hdr() can also not be used.

Best regards
Christian