[PATCH net-next v5 0/2] net: hsr: strict supervision TLV validation

[PATCH net-next v5 0/2] net: hsr: strict supervision TLV validation

[luka.gejak]

From: Luka Gejak <luka.gejak@linux.dev>

Changes in v5:
 - Reverted TLV loop in Patch 1 to strict sequential parsing per IEC
   62439-3.
 - Retained pskb_may_pull() logic to ensure memory safety for TLV
   headers.
 - Dropped Reviewed-by from Patch 1 due to the logic evolving since
   original review.
 - Added Assisted-by tag for AI-aided translation and formatting to
   both patches.

Changes in v4:
 - Split from a 4-patch series into 'net' and 'net-next' as requested.
 - Implemented a TLV walker in Patch 1 to correctly handle extension
   TLVs and avoid regressions on paged frames/non-linearized skbs.
 - Corrected pskb_may_pull() logic to include the TLV header size.

History of pre-separation series (v1-v3):
Changes in v3:
 - addressed Felix review feedback in the VLAN add unwind fix
 - removed the superfluous empty line

Changes in v2:
 - picked up Reviewed-by tags on patches 1, 3 and 4
 - changes in patch 2 per advice of Felix Maurer

Luka Gejak (2):
  net: hsr: require valid EOT supervision TLV
  net: hsr: reject unresolved interlink ifindex

 net/hsr/hsr_forward.c | 20 +++++++++-----------
 net/hsr/hsr_netlink.c |  7 ++++++-
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.53.0

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

[luka.gejak]

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.

Assisted-by: Gemini:Gemini-3.1-flash
Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
---
 net/hsr/hsr_forward.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

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 */
 	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;
 
-- 
2.53.0

[PATCH net-next v5 2/2] net: hsr: reject unresolved interlink ifindex

[luka.gejak]

From: Luka Gejak <luka.gejak@linux.dev>

In hsr_newlink(), a provided but invalid IFLA_HSR_INTERLINK attribute
was silently ignored if __dev_get_by_index() returned NULL. This leads
to incorrect RedBox topology creation without notifying the user.

Fix this by returning -EINVAL and an extack message when the
interlink attribute is present but cannot be resolved.

Assisted-by: Gemini:Gemini-3.1-flash
Reviewed-by: Felix Maurer <fmaurer@redhat.com>
Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
---
 net/hsr/hsr_netlink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/hsr/hsr_netlink.c b/net/hsr/hsr_netlink.c
index db0b0af7a692..f0ca23da3ab9 100644
--- a/net/hsr/hsr_netlink.c
+++ b/net/hsr/hsr_netlink.c
@@ -76,9 +76,14 @@ static int hsr_newlink(struct net_device *dev,
 		return -EINVAL;
 	}
 
-	if (data[IFLA_HSR_INTERLINK])
+	if (data[IFLA_HSR_INTERLINK]) {
 		interlink = __dev_get_by_index(link_net,
 					       nla_get_u32(data[IFLA_HSR_INTERLINK]));
+		if (!interlink) {
+			NL_SET_ERR_MSG_MOD(extack, "Interlink does not exist");
+			return -EINVAL;
+		}
+	}
 
 	if (interlink && interlink == link[0]) {
 		NL_SET_ERR_MSG_MOD(extack, "Interlink and Slave1 are the same");
-- 
2.53.0

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

[Fernando Fernandez Mancera]

On 4/7/26 6:25 PM, 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.
> 
> Assisted-by: Gemini:Gemini-3.1-flash
> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
> ---
>   net/hsr/hsr_forward.c | 20 +++++++++-----------
>   1 file changed, 9 insertions(+), 11 deletions(-)
> 
> 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 */
>   	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)))

Hi Luka,

why is the length adjusted here? The current total length was adjusted 
correctly above see:

total_length += sizeof(struct hsr_sup_tlv) + hsr_sup_tlv->HSR_TLV_length;

Thanks,
Fernando.

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

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

[Fernando Fernandez Mancera]

On 4/8/26 9:48 AM, Fernando Fernandez Mancera wrote:
> On 4/7/26 6:25 PM, 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.
>>
>> Assisted-by: Gemini:Gemini-3.1-flash
>> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
>> ---
>>   net/hsr/hsr_forward.c | 20 +++++++++-----------
>>   1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> 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 */
>>       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)))
> 
> Hi Luka,
> 
> why is the length adjusted here? The current total length was adjusted 
> correctly above see:
> 

Never mind, it makes sense as the total_length must point to the 
beginning of the next TLV.

LGTM,

Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>

Thanks,
Fernando.

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

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

[Luka Gejak]

On April 8, 2026 10:05:31 AM GMT+02:00, Fernando Fernandez Mancera <fmancera@suse.de> wrote:
>On 4/8/26 9:48 AM, Fernando Fernandez Mancera wrote:
>> On 4/7/26 6:25 PM, 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.
>>> 
>>> Assisted-by: Gemini:Gemini-3.1-flash
>>> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>
>>> ---
>>>   net/hsr/hsr_forward.c | 20 +++++++++-----------
>>>   1 file changed, 9 insertions(+), 11 deletions(-)
>>> 
>>> 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 */
>>>       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)))
>> 
>> Hi Luka,
>> 
>> why is the length adjusted here? The current total length was adjusted correctly above see:
>> 
>
>Never mind, it makes sense as the total_length must point to the beginning of the next TLV.
>
>LGTM,
>
>Reviewed-by: Fernando Fernandez Mancera <fmancera@suse.de>
>
>Thanks,
>Fernando.
>
Hi Fernando,
Thank you for the follow-up and the reviewed-by tag.
I was actually just about to send an explanation regarding the 
pskb_may_pull() logic, but that seems unnecessary now. I'm glad it 
makes sense on your end as well.
Best regards,
Luka
>>>               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;
>> 
>

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

[Felix Maurer]

On Tue, Apr 07, 2026 at 06:25:01PM +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.
>
> Assisted-by: Gemini:Gemini-3.1-flash
> Signed-off-by: Luka Gejak <luka.gejak@linux.dev>

Thanks for the patch, including making sure that there is enough data
available to read the next TLVs.

Reviewed-by: Felix Maurer <fmaurer@redhat.com>

> ---
>  net/hsr/hsr_forward.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>
> 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 */
>  	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;
>
> --
> 2.53.0
>

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

[Jakub Kicinski]

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?
-- 
pw-bot: cr

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

[Luka Gejak]

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

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

[Jakub Kicinski]

On Sun, 12 Apr 2026 22:13:35 +0200 Luka Gejak wrote:
> 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.

That's not really what I had in mind. I was thinking of a loop which
just skips the TLVs in order, leaving the parsing of known TLVs as is.
But I've never used HSR maybe this sort of strict validation is somehow
okay in HSR deployments.

Please just undo the comment tweaks then.

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

[Luka Gejak]

On April 12, 2026 10:31:57 PM GMT+02:00, Jakub Kicinski <kuba@kernel.org> wrote:
>On Sun, 12 Apr 2026 22:13:35 +0200 Luka Gejak wrote:
>> 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.
>
>That's not really what I had in mind. I was thinking of a loop which
>just skips the TLVs in order, leaving the parsing of known TLVs as is.
>But I've never used HSR maybe this sort of strict validation is somehow
>okay in HSR deployments.
>
>Please just undo the comment tweaks then.

So keep other changes as is and only undo comment changes?

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

[Jakub Kicinski]

On Sun, 12 Apr 2026 22:46:48 +0200 Luka Gejak wrote:
> On April 12, 2026 10:31:57 PM GMT+02:00, Jakub Kicinski <kuba@kernel.org> wrote:
> >On Sun, 12 Apr 2026 22:13:35 +0200 Luka Gejak wrote:  
> >> 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.  
> >
> >That's not really what I had in mind. I was thinking of a loop which
> >just skips the TLVs in order, leaving the parsing of known TLVs as is.
> >But I've never used HSR maybe this sort of strict validation is somehow
> >okay in HSR deployments.
> >
> >Please just undo the comment tweaks then.  
> 
> So keep other changes as is and only undo comment changes?

Yes, how is that not clear from my previous message? :|

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

[Felix Maurer]

On Sun, Apr 12, 2026 at 01:31:57PM -0700, Jakub Kicinski wrote:
> On Sun, 12 Apr 2026 22:13:35 +0200 Luka Gejak wrote:
> > 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.
>
> That's not really what I had in mind. I was thinking of a loop which
> just skips the TLVs in order, leaving the parsing of known TLVs as is.
> But I've never used HSR maybe this sort of strict validation is somehow
> okay in HSR deployments.

Hi Jakub,

I'm chiming in here as I was one of the reviewers asking for the strict
validation. The HSR supervision frames have this TLV structure that may
appear to support optionals or (unknown) extensions of some kind. But
the standard just has two potential frame formats (for normal and RedBox
supervision frames), both of them completely specified. Also, the
supervision frames have a version field in the beginning. IMHO, this
leaves no room to put other TLVs there. Therefore, I don't think we gain
anything but unexpected behavior if we start accepting frames with
arbitrary additional TLVs/data.

Thanks,
   Felix