Re: [PATCH net-next v8 3/8] hsr: Add a magic header for sending PTP packets

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] hsr_dev_xmit() (net/hsr/hsr_device.c) now drops every skb that
  has skb->protocol == htons(ETH_P_1588) unless the first 14 bytes form…
- [Medium] hsr_dev_xmit() treats the first 14 bytes of any skb with
  skb->protocol == ETH_P_1588 as trusted in-band control metadata.
- [Low] At this commit, the new uAPI fields tx_port and hsr_hdr
  (include/uapi/linux/hsr_ptp.h) are parsed and checked in…
- [Low] At this commit, the user-supplied hsr_hdr flag sets mac_len to 20
  without checking that the inner frame really has an HSR tag at offset…

--

hsr: Add a magic header for sending PTP packets

This patch adds a uAPI struct, hsr_inline_header. It must sit in front
of every ETH_P_1588 frame sent through the HSR master device.
hsr_dev_xmit() checks the magic, reads the requested tx port and the
"already has HSR header" flag, strips the inline header and passes both
values to hsr_forward_skb(), which will use them in a later patch.

> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 59df647d2fd45..72347477f4f21 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -223,24 +223,66 @@ static netdev_features_t hsr_fix_features(struct net_device *dev,

[ ... ]

> +	skb->dev = master->dev;
> +	if (skb->protocol == htons(ETH_P_1588)) {

[Severity: Medium]
Does this drop every PTP frame that lacks the new inline header?

The gate looks only at skb->protocol, so it also catches senders that
know nothing about struct hsr_inline_header. With a normal Ethernet
header, the magic check below reads bytes 0-3 of the source MAC. These
frames then go to the drop label, and only tx_dropped is incremented.

Before this patch, hsr_forward_skb() forwarded these frames to both
slaves. Some cases that look affected:

  - AF_PACKET SOCK_DGRAM senders. eth_header() builds the header, so the
    device MAC sits where the magic is expected.

  - PTP frames that an L2 forwarder sends into hsr0, for example with
    hsr0 as a bridge port via br_add_if(). eth_type_trans() has already
    set skb->protocol to ETH_P_1588.

  - PTP sent through a VLAN upper of hsr0. hsr_dev_setup() advertises
    NETIF_F_HW_VLAN_CTAG_TX, so the tag stays in skb metadata and
    skb->protocol is still ETH_P_1588.

The commit message says:

    Sending PTP packets (ETH_P_1588) via the HSR stack is pointless in its
    current shape because the requested PTP timestamp is not routed to the
    sender.

That covers local senders that ask for TX timestamps. Does it also hold
for transit PTP traffic, or for PTP traffic that doesn't use
timestamps?

The commit message calls the header mandatory. It doesn't mention that
existing senders without the header now lose their traffic.
hsr_dev_xmit() isn't changed later in the series, so this stays in place.

> +		struct hsr_inline_header *hsr_opt;
> +		struct ethhdr *eth_hdr;
> +		unsigned int hdr_len;
> +
> +		BUILD_BUG_ON(sizeof(struct hsr_inline_header) != sizeof(struct ethhdr));
> +
> +		/* need to access the magic header */
> +		if (!pskb_may_pull(skb, sizeof(struct hsr_inline_header)))
> +			goto drop;
> +
> +		hsr_opt = (struct hsr_inline_header *)skb_mac_header(skb);
> +		if (hsr_opt->magic != htonl(HSR_INLINE_HDR))
> +			goto drop;

[Severity: Medium]
Can a remote host provide this control header?

The only check is a constant magic in the source MAC position, and the
struct maps directly onto dst MAC, src MAC and ethertype.

Some frames are received from the wire and forwarded into hsr0 by tc
mirred (ingress to egress) or by an OVS output action. They keep
skb->protocol = ETH_P_1588 from eth_type_trans(). Their first 14 bytes
are the Ethernet header that the remote sender chose.

Consider a frame with:

  - dst MAC {01 or 02, hsr_hdr, 00, 00, 00, 00}
  - src MAC {af:48:53:52, 00, 00}
  - ethertype 0x88f7

It passes this check. The bridge rejects it because the source is
multicast, but tc mirred and OVS don't. OVS and promiscuous NICs accept
the crafted destination MAC.

hsr_dev_xmit() then strips those 14 bytes. It sends the inner frame on
the port the sender picked, with the hsr_hdr flag the sender chose.

The later commit "hsr: Use the port and header information in
hsr_forward_skb()" makes has_hsr_header skip several steps:

  - hsr_get_node()
  - hsr_register_frame_in()
  - sequence number assignment
  - source MAC substitution in hsr_xmit()

As a result, the frame goes out unchanged with a spoofed source MAC,
HSR sequence number and path id.

Without this patch, forwarded frames are always re-tagged with the local
node's sequence number and source MAC. Forwarded ETH_P_HSR frames are
rejected by the mac_len < sizeof(struct hsr_ethhdr) check, because
hsr_dev_xmit() sets mac_len to 14.

Could this be used to poison the duplicate-discard tables of other ring
nodes, so that legitimate traffic from the spoofed node gets dropped?

> +
> +		has_header = hsr_opt->hsr_hdr;
> +		tx_port = hsr_opt->tx_port;
> +		if (tx_port != HSR_PT_SLAVE_A && tx_port != HSR_PT_SLAVE_B)
> +			goto drop;
> +
> +		if (has_header)
> +			hdr_len = ETH_HLEN + HSR_HLEN;
> +		else
> +			hdr_len = ETH_HLEN;
> +
> +		/* Ensure the real header after the stripped one can be
> +		 * accessed
> +		 */
> +		if (!pskb_may_pull(skb, sizeof(struct hsr_inline_header) +
> +				   hdr_len))
> +			goto drop;
> +
> +		eth_hdr = skb_pull(skb, sizeof(struct hsr_inline_header));
> +		skb_set_network_header(skb, hdr_len);
> +		skb->protocol = eth_hdr->h_proto;

[Severity: Low]
Should hsr_hdr be checked against the inner frame before it is trusted
here?

With hsr_hdr set, mac_len becomes ETH_HLEN + HSR_HLEN. Nothing checks
that eth_hdr->h_proto is really an HSR ethertype.

Take a VLAN-tagged frame with an encapsulated ETH_P_HSR and hsr_hdr = 1.
It gets past this guard in hsr_fill_frame_info(), which would otherwise
reject it:

	if (skb->mac_len < sizeof(struct hsr_ethhdr))
		return -EINVAL;

Then hsr_get_skb_sequence_nr() and hsr_set_path_id() read and write the
tag at mac_header + 14, which is inside the VLAN header, instead of at
mac_header + 18.

The pskb_may_pull() above keeps these accesses within the linear data.

The later commit "hsr: Use the port and header information in
hsr_forward_skb()" makes hsr_fill_frame_info() return early when
has_foreign_header is set. hsr_forward_do() then sends a clone of
skb_std without calling hsr_set_path_id(). So this affects only this
intermediate commit.

[ ... ]

> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index 73bc4b8950071..4afe9e1d65b7e 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -773,7 +773,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>  }
>  
>  /* Must be called holding rcu read lock (because of the port parameter) */
> -void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port)
> +void hsr_forward_skb(struct sk_buff *skb, struct hsr_port *port,
> +		     enum hsr_port_type tx_port, bool has_hsr_header)
>  {
>  	struct hsr_frame_info frame;
>  

[Severity: Low]
At this commit, hsr_forward_skb() accepts tx_port and has_hsr_header but
doesn't use them. The body still does:

	if (fill_frame_info(&frame, skb, port) < 0)
		goto out_drop;

hsr_forward_do() still sends to both slaves. This patch already exports
struct hsr_inline_header as uAPI. At this point in the series, a frame
that asks for a single port is still sent on both ports.

The commit message says the values "will be used later". The follow-up
commit "hsr: Use the port and header information in hsr_forward_skb()"
adds frame->req_tx_port and frame->has_foreign_header. It acts on them in
fill_frame_info(), hsr_fill_frame_info(), hsr_forward_do() and
hsr_xmit(). So this only matters when bisecting onto this commit.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261002-hsr_ptp-v8-0-60dabc07e554%40linutronix.de