netdev - Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function

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: <87a6zli04l.fsf@mellanox.com>
Date:   Mon, 27 Jul 2020 15:00:10 +0200
From:   Petr Machata <petrm@...lanox.com>
To:     Kurt Kanzenbach <kurt@...utronix.de>
Cc:     Richard Cochran <richardcochran@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Ido Schimmel <idosch@...lanox.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>,
        Samuel Zou <zou_wei@...wei.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 4/9] mlxsw: spectrum_ptp: Use generic helper function


Kurt Kanzenbach <kurt@...utronix.de> writes:

> @@ -329,30 +327,14 @@ static int mlxsw_sp_ptp_parse(struct sk_buff *skb,
>  		return -ERANGE;
>  	}
>
> -	if (ptp_class & PTP_CLASS_VLAN)
> -		offset += VLAN_HLEN;
> -
> -	switch (ptp_class & PTP_CLASS_PMASK) {
> -	case PTP_CLASS_IPV4:
> -		offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN;
> -		break;
> -	case PTP_CLASS_IPV6:
> -		offset += ETH_HLEN + IP6_HLEN + UDP_HLEN;
> -		break;
> -	case PTP_CLASS_L2:
> -		offset += ETH_HLEN;
> -		break;
> -	default:
> -		return -ERANGE;
> -	}
> -
> -	/* PTP header is 34 bytes. */
> -	if (skb->len < offset + 34)
> +	hdr = ptp_parse_header(skb, ptp_class);
> +	if (!hdr)
>  		return -EINVAL;

So this looks good, and works, but I'm wondering about one thing.

Your code (and evidently most drivers as well) use a different check
than mlxsw, namely skb->len + ETH_HLEN < X. When I print_hex_dump()
skb_mac_header(skb), skb->len in mlxsw with some test packet, I get e.g.
this:

    00000000259a4db7: 01 00 5e 00 01 81 00 02 c9 a4 e4 e1 08 00 45 00  ..^...........E.
    000000005f29f0eb: 00 48 0d c9 40 00 01 11 c8 59 c0 00 02 01 e0 00  .H..@....Y......
    00000000f3663e9e: 01 81 01 3f 01 3f 00 34 9f d3 00 02 00 2c 00 00  ...?.?.4.....,..
                            ^sp^^ ^dp^^ ^len^ ^cks^       ^len^
    00000000b3914606: 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02  ................
    000000002e7828ea: c9 ff fe a4 e4 e1 00 01 09 fa 00 00 00 00 00 00  ................
    000000000b98156e: 00 00 00 00 00 00                                ......

Both UDP and PTP length fields indicate that the payload ends exactly at
the end of the dump. So apparently skb->len contains all the payload
bytes, including the Ethernet header.

Is that the case for other drivers as well? Maybe mlxsw is just missing
some SKB magic in the driver.