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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-10-06 14:31:15 [+0000], netdev-bot+sashiko@kernel.org wrote:
> > +	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?

Yes. Intended.

…
> 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?

PTP traffic without timestamps does not sound like accurate usage of
PTP.

> 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.

Mandatory implies this.

> > +		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

This shouldn't happen. This injection/ magic header is in hsr_dev_xmit()
and only for the HSR_PT_MASTER port which is the hsr0 interface. So it
does not happen on received packets which would be the HSR_PT_SLAVE_{AB}
interface. So no.

…
> [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.

If this only effects intermediate commit, then it should be fine.

> [ ... ]
> 
> > 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:
…
> 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.

This is fine as-is.

Sebastian