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]

Date:   Wed, 29 Jul 2020 15:23:27 +0300
From:   Grygorii Strashko <grygorii.strashko@...com>
To:     Petr Machata <petrm@...lanox.com>,
        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>,
        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



On 29/07/2020 00:06, Petr Machata wrote:
> 
> Kurt Kanzenbach <kurt@...utronix.de> writes:
> 
>> On Mon Jul 27 2020, Petr Machata wrote:
>>> So this looks good, and works, but I'm wondering about one thing.
>>
>> Thanks for testing.
>>
>>>
>>> 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.
>>
>> So I run some tests (on other hardware/drivers) and it seems like that
>> the skb->len usually doesn't include the ETH_HLEN. Therefore, it is
>> added to the check.
>>
>> Looking at the driver code:
>>
>> |static void mlxsw_sp_rx_sample_listener(struct sk_buff *skb, u8 local_port,
>> |					void *trap_ctx)
>> |{
>> |	[...]
>> |	/* The sample handler expects skb->data to point to the start of the
>> |	 * Ethernet header.
>> |	 */
>> |	skb_push(skb, ETH_HLEN);
>> |	mlxsw_sp_sample_receive(mlxsw_sp, skb, local_port);
>> |}
>>
>> Maybe that's the issue here?
> 
> Correct, mlxsw pushes the header very soon. Given that both
> ptp_classify_raw() and eth_type_trans() that are invoked later assume
> the header, it is reasonable. I have shuffled the pushes around and have
> a patch that both works and I think is correct.
> 
> However, I find it odd that ptp_classify_raw() assumes that ->data
> points at Ethernet, while ptp_parse_header() makes the contrary
> assumption that ->len does not cover Ethernet. Those functions are
> likely to be used just a couple calls away from each other, if not
> outright next to each other.
> 
> I suspect that ti/cpts.c and ti/am65-cpts.c (patches 5 and 6) actually
> hit an issue in this. ptp_classify_raw() is called without a surrounding
> _push / _pull (unlike DSA), which would imply skb->data points at
> Ethernet header, and indeed, the way the "data" variable is used
> confirms it.

Both drivers, in all cases, will get
  ->data points at Ethernet header and
  ->len covers ETH_HLEN

So, yes below check is incorrect, in general, and will be false always if other
calculation are correct
(only IPV4_HLEN(data + offset) can cause issues).

if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid))
		return 0;

it might be good to update ptp_parse_header() documentation with expected state of skb.

  (At the same time the code adds ETH_HLEN to skb->len, but
> maybe it is just a cut'n'paste.) But then ptp_parse_header() is called,
> and that makes the assumption that skb->len does not cover the Ethernet
> header.
> 
>> I was also wondering about something else in that driver driver: The
>> parsing code allows for ptp v1, but the message type was always fetched
>> from offset 0 in the header. Is that indented?
> 
> Yeah, I noticed that as well. That was a bug in the mlxsw code. Good
> riddance :)
> 

-- 
Best regards,
grygorii