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

[Fernando Fernandez Mancera <fmancera@suse.de>]

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

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.

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

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

> 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.
> 
> Let me know if you'd prefer I drop the loop for v5.
> Best regards,
> Luka Gejak
>