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

["Luka Gejak" <luka.gejak@linux.dev>]

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.

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