Re: [PATCH net-next v4 1/2] net: hsr: require valid EOT supervision TLV

[Felix Maurer <fmaurer@redhat.com>]

On Thu, Apr 02, 2026 at 01:53:14AM +0200, Fernando Fernandez Mancera wrote:
> On 4/1/26 6:59 PM, Luka Gejak wrote:
> > On Wed Apr 1, 2026 at 4:47 PM CEST, Fernando Fernandez Mancera wrote:
> > > On 4/1/26 11:23 AM, luka.gejak@linux.dev wrote:
> > > > From: Luka Gejak <luka.gejak@linux.dev>
> > > >
> > > > Supervision frames are only valid if terminated with a zero-length EOT
> > > > TLV. The current check fails to reject non-EOT entries as the terminal
> > > > TLV, potentially allowing malformed supervision traffic.
> > > >
> > > > Fix this by strictly requiring the terminal TLV to be HSR_TLV_EOT
> > > > with a length of zero.
> > > >
> > > > Reviewed-by: Felix Maurer <fmaurer@redhat.com>
> > > > Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
> > > > ---
> > > >    net/hsr/hsr_forward.c | 41 ++++++++++++++++++++++-------------------
> > > >    1 file changed, 22 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > > > index 0aca859c88cb..17b705235c4a 100644
> > > > --- a/net/hsr/hsr_forward.c
> > > > +++ b/net/hsr/hsr_forward.c
> > > > @@ -82,39 +82,42 @@ static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb)
> > > >    	    hsr_sup_tag->tlv.HSR_TLV_length != sizeof(struct hsr_sup_payload))
> > > >    		return false;
> > > > -	/* Get next tlv */
> > > > +	/* Advance past the first TLV payload to reach next TLV header */
> > > >    	total_length += hsr_sup_tag->tlv.HSR_TLV_length;
> > > > -	if (!pskb_may_pull(skb, total_length))
> > > > +	/* Linearize next TLV header before access */
> > > > +	if (!pskb_may_pull(skb, total_length + sizeof(struct hsr_sup_tlv)))
> > > >    		return false;
> > > >    	skb_pull(skb, total_length);
> > > >    	hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
> > > >    	skb_push(skb, total_length);
> > > > -	/* if this is a redbox supervision frame we need to verify
> > > > -	 * that more data is available
> > > > +	/* Walk through TLVs to find end-of-TLV marker, skipping any unknown
> > > > +	 * extension TLVs to maintain forward compatibility.
> > > >    	 */
> > > > -	if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
> > > > -		/* tlv length must be a length of a mac address */
> > > > -		if (hsr_sup_tlv->HSR_TLV_length != sizeof(struct hsr_sup_payload))
> > > > -			return false;
> > > > +	for (;;) {
> > > > +		if (hsr_sup_tlv->HSR_TLV_type == HSR_TLV_EOT &&
> > > > +		    hsr_sup_tlv->HSR_TLV_length == 0)
> > > > +			return true;
> > >
> > > I do not follow this approach, why a loop? From IEC 62439-3, I do not
> > > understand that supervision frames could have multiple
> > > PRP_TLV_REDBOX_MAC TLVs. The current code handles the TLVs correctly.
> > >
> > > Which makes me wonder, how are you testing this? Do you have some
> > > hardware with HSR/PRP support that is sending these frames? If so, which
> > > one? Are you testing this using a HSR/PRP environment with purely Linux
> > > devices?
> > >
> > > Thanks,
> > > Fernando.
> > >
> > > > -		/* make sure another tlv follows */
> > > > -		total_length += sizeof(struct hsr_sup_tlv) + hsr_sup_tlv->HSR_TLV_length;
> > > > -		if (!pskb_may_pull(skb, total_length))
> > > > +		/* Validate known TLV types */
> > > > +		if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
> > > > +			if (hsr_sup_tlv->HSR_TLV_length !=
> > > > +			    sizeof(struct hsr_sup_payload))
> > > > +				return false;
> > > > +		}
> > > > +
> > > > +		/* Advance past current TLV: header + payload */
> > > > +		total_length += sizeof(struct hsr_sup_tlv) +
> > > > +				hsr_sup_tlv->HSR_TLV_length;
> > > > +		/* Linearize next TLV header before access */
> > > > +		if (!pskb_may_pull(skb,
> > > > +				   total_length + sizeof(struct hsr_sup_tlv)))
> > > >    			return false;
> > > > -		/* get next tlv */
> > > >    		skb_pull(skb, total_length);
> > > >    		hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
> > > >    		skb_push(skb, total_length);
> > > >    	}
> >
> > Hi Fernando,
> >
> > You are right that IEC 62439-3 does not specify multiple
> > PRP_TLV_REDBOX_MAC TLVs. My intention with the loop was not to handle
> > multiple RedBox MACs, but rather to make the parser robust against
> > unknown TLV types. If a future revision of the standard or a vendor
> > extension introduces a new TLV, the loop allows the kernel to safely
> > skip over unrecognized TLVs by reading their length, ensuring it can
> > still validate the HSR_TLV_EOT marker at the end.

Hi Fernando, Luka,

> AFAIU, the TLVs must be in the right order. I don't know, it doesn't sound
> very convincing to me that we are anticipating to new TLVs. HSR/PRP isn't a
> very active protocol and it has few users in Kernel probably compare to
> other protocols because it is used in a very specific industry domain.

I agree with this. IMHO, the supervision frames should look the way they
are described in the standard, i.e., the TLVs ordered as the current
code expects them.

Luka, I'm not sure what your stance is to that? On one hand, you are
proposing a patch making the checks more strict (TLV_END && length ==
0), on the other hand you are proposing a patch accepting supervision
frames with random new or proprietary TLVs.

> If a new revision of the protocol specs is released we can always update our
> implementation.

This, especially because the supervision frames have an explicit version
number field (something we don't check at the moment, but probably
should in the future).

> Anyway, since Felix reviewed the initial patch let's wait for his review.

I'll add more notes in another reply, directly to the patch.

> > However, if the preference for the HSR subsystem is strict adherence to
> > only currently defined TLVs over forward compatibility, I completely
> > understand.
> >
> > Furthermore, I am testing this using a purely Linux environment by
> > using a virtual HSR environment on Arch Linux. I set up two network
> > namespaces connected via veth pairs and instantiated HSR interfaces.
> >
> > The nodes successfully synchronized and maintained the connection. I
> > confirmed this by observing the expected duplicate packets (DUP!)
> > during ping tests between namespaces and by verifying that supervision
> > frames were correctly parsed, allowing the nodes to populate their
> > remote node tables.

Side note: hsr exposes the node table through debugfs, that's maybe a
better way to test/verify such things.

Thanks,
   Felix