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

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

On April 12, 2026 9:45:58 PM GMT+02:00, Jakub Kicinski <kuba@kernel.org> wrote:
>On Tue,  7 Apr 2026 18:25:01 +0200 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, and properly linearizing the TLV header before
>> access.
>
>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>> index 0aca859c88cb..eb89cc44eac0 100644
>> --- a/net/hsr/hsr_forward.c
>> +++ b/net/hsr/hsr_forward.c
>> @@ -82,35 +82,33 @@ 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 */
>> +	/* Get next TLV */
>
>The capitalization of the comments makes the real changes in this patch
>harder to spot and follow. Please don't tweak the comments unless you're
>really changing / improving them.
>
>>  	total_length += hsr_sup_tag->tlv.HSR_TLV_length;
>> -	if (!pskb_may_pull(skb, total_length))
>> +	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
>> -	 */
>> +	/* If this is a RedBox supervision frame, verify additional data */
>>  	if (hsr_sup_tlv->HSR_TLV_type == PRP_TLV_REDBOX_MAC) {
>> -		/* tlv length must be a length of a mac address */
>> +		/* TLV length must be the size of a MAC address */
>>  		if (hsr_sup_tlv->HSR_TLV_length != sizeof(struct hsr_sup_payload))
>>  			return false;
>>  
>> -		/* make sure another tlv follows */
>> +		/* 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))
>> +		if (!pskb_may_pull(skb, total_length + sizeof(struct hsr_sup_tlv)))
>>  			return false;
>>  
>> -		/* get next tlv */
>> +		/* Get next TLV */
>>  		skb_pull(skb, total_length);
>>  		hsr_sup_tlv = (struct hsr_sup_tlv *)skb->data;
>>  		skb_push(skb, total_length);
>>  	}
>>  
>> -	/* end of tlvs must follow at the end */
>> -	if (hsr_sup_tlv->HSR_TLV_type == HSR_TLV_EOT &&
>> +	/* Supervision frame must end with EOT TLV */
>> +	if (hsr_sup_tlv->HSR_TLV_type != HSR_TLV_EOT ||
>>  	    hsr_sup_tlv->HSR_TLV_length != 0)
>>  		return false;
>
>Aren't there more optional TLVs that we don't support?
>You mentioned making sure that the final TLV is a zero-length EOT
>but this check is far stricter.
>
>Should we replace this check with a loop which skips over TLVs
>until EOT is reached?

Hi Jakub,
Thank you for the feedback.
I apologize for the noise in the comments. I will revert all cosmetic 
capitalization changes in v6 to keep the diff focused on the logic.
Regarding the TLV loop: I actually implemented a TLV walker in v4 [1] 
for this exact reason, but I moved to strict sequential parsing in v5 
based on reviewer's feedback to keep the implementation simple. Could 
you please check if the approach used in v4 is what you had in mind? 
If so, I will rebase that logic onto the memory safety fixes 
(pskb_may_pull) from v5 and submit it as v6.

[1] https://lore.kernel.org/netdev/20260401092324.52266-2-luka.gejak@linux.dev/

Best regards,
Luka Gejak