net: hsr: strict supervision TLV validation

PATCH Superseded [email protected] 4 participants 13 messages Apr 7 – Apr 13

Subsystems (3)

Networking Network drivers HSR network protocol

lore

Share & tools

lore

[email protected]

author

2026-04-07 16:25 UTC #0 lore raw

v5 cover net: hsr: strict supervision TLV validation

From: Luka Gejak <[email protected]>

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(-)

Signature · 1 line

2.53.0

[email protected]

author

2026-04-07 16:25 UTC #1 lore raw

PATCH v5 1/2 net: hsr: require valid EOT supervision TLV

From: Luka Gejak <[email protected]>

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 <[email protected]>

---
 net/hsr/hsr_forward.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

net/hsr/hsr_forward.c +9 ~~-11~~

index 0aca859c88cb..eb89cc44eac0 100644
@@ -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;

Signature · 1 line

2.53.0

Fernando Fernandez Mancera

2026-04-08 07:48 UTC #2 lore raw

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

Quoted 39 lines · On 4/7/26 6:25 PM, [email protected] wrote:

From: Luka Gejak <[email protected]>
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 <[email protected]>
---
  net/hsr/hsr_forward.c | 20 +++++++++-----------
  1 file changed, 9 insertions(+), 11 deletions(-)
net/hsr/hsr_forward.c +3 -5
index 0aca859c88cb..eb89cc44eac0 100644
@@ -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) {

net/hsr/hsr_forward.c static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) · line 101

Quoted 10 lines

net/hsr/hsr_forward.c +3 -3

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

Quoted 16 lines

net/hsr/hsr_forward.c +3 -3

  			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;

Fernando Fernandez Mancera

Reviewed-by

2026-04-08 08:05 UTC #3 lore raw

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

Quoted 46 lines · Fernando Fernandez Mancera

Quoted 45 lines · On 4/7/26 6:25 PM, [email protected] wrote:

From: Luka Gejak <[email protected]>

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 <[email protected]>

---
net/hsr/hsr_forward.c | 20 +++++++++-----------

  1 file changed, 9 insertions(+), 11 deletions(-)

net/hsr/hsr_forward.c +5 -7

index 0aca859c88cb..eb89cc44eac0 100644
@@ -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-

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
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 <[email protected]>

Thanks,
Fernando.

Quoted 14 lines

Quoted 13 lines
              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;

Luka Gejak

author

2026-04-08 08:19 UTC #4 lore raw

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

Quoted 54 lines · Fernando Fernandez Mancera

Quoted 50 lines · Fernando Fernandez Mancera

Quoted 36 lines · On 4/7/26 6:25 PM, [email protected] wrote:

From: Luka Gejak <[email protected]>

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 <[email protected]>

---
net/hsr/hsr_forward.c | 20 +++++++++-----------

  1 file changed, 9 insertions(+), 11 deletions(-)

net/hsr/hsr_forward.c +3 -5

index 0aca859c88cb..eb89cc44eac0 100644
@@ -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 */

net/hsr/hsr_forward.c static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) · line 98

Quoted 10 lines

net/hsr/hsr_forward.c +3 -3

       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,

Quoted 2 lines

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

Quoted 9 lines

Never mind, it makes sense as the total_length must point to the beginning of the next TLV.
LGTM,
Reviewed-by Fernando Fernandez Mancera <[email protected]>
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

Quoted 15 lines

Quoted 14 lines

Quoted 13 lines

net/hsr/hsr_forward.c +3 -3

               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;

Felix Maurer

Reviewed-by on v5 1/2

2026-04-08 09:32 UTC #5 lore raw

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

In reply to [email protected]

Quoted 6 lines · On Tue, Apr 07, 2026 at 06:25:01PM +0200, [email protected] wrote:

From: Luka Gejak <[email protected]>
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.

Quoted 6 lines

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 <[email protected]>

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

Reviewed-by Felix Maurer <[email protected]>

Quoted 56 lines

---
 net/hsr/hsr_forward.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

net/hsr/hsr_forward.c +9 ~~-12~~

index 0aca859c88cb..eb89cc44eac0 100644
@@ -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

Jakub Kicinski

maintainer

2026-04-12 19:45 UTC #6 lore raw

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

In reply to [email protected]

Quoted 15 lines · On Tue, 7 Apr 2026 18:25:01 +0200 [email protected] wrote:

From: Luka Gejak <[email protected]>
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.
net/hsr/hsr_forward.c +0 -0
index 0aca859c88cb..eb89cc44eac0 100644

net/hsr/hsr_forward.c static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) · line 85

Quoted 5 lines

net/hsr/hsr_forward.c +1 -1

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

Quoted 28 lines

net/hsr/hsr_forward.c +6 -8

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

net/hsr/hsr_forward.c static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) · line 113

Quoted 10 lines

net/hsr/hsr_forward.c +2 -2

 		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?

Signature · 1 line

pw-bot: cr

Luka Gejak

author

2026-04-12 20:13 UTC #7 lore raw

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

Quoted 69 lines · Jakub Kicinski

Quoted 15 lines · On Tue, 7 Apr 2026 18:25:01 +0200 [email protected] wrote:
From: Luka Gejak <[email protected]>
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.
net/hsr/hsr_forward.c +0 -0
index 0aca859c88cb..eb89cc44eac0 100644
net/hsr/hsr_forward.c static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) · line 85
Quoted 5 lines
net/hsr/hsr_forward.c +1 -1
@@ -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.
Quoted 28 lines
net/hsr/hsr_forward.c +6 -8
 	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);
net/hsr/hsr_forward.c static bool is_supervision_frame(struct hsr_priv *hsr, struct sk_buff *skb) · line 113
Quoted 10 lines
net/hsr/hsr_forward.c +2 -2
 		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.

Quoted 2 lines

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/[email protected]/

Best regards,
Luka Gejak

Jakub Kicinski

maintainer

2026-04-12 20:31 UTC #8 lore raw

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

Quoted 6 lines · 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.

Luka Gejak

author

2026-04-12 20:47 UTC #9 lore raw

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

Quoted 8 lines · Jakub Kicinski

Quoted 6 lines · 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.

Quoted 6 lines

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?

Jakub Kicinski

maintainer

2026-04-12 21:13 UTC #10 lore raw

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

Quoted 9 lines · On Sun, 12 Apr 2026 22:46:48 +0200 Luka Gejak wrote:

Quoted 8 lines · Jakub Kicinski
Quoted 6 lines · 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.

Quoted 8 lines

Quoted 6 lines
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? :|

Felix Maurer

2026-04-13 09:14 UTC #11 lore raw

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

In reply to Jakub Kicinski

Quoted 8 lines · Jakub Kicinski

Quoted 6 lines · 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.

Quoted 4 lines

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

[email protected]

author

2026-04-07 16:25 UTC #12 lore raw

PATCH v5 2/2 net: hsr: reject unresolved interlink ifindex

In reply to [email protected]

From: Luka Gejak <[email protected]>

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 <[email protected]>

Signed-off-by Luka Gejak <[email protected]>

---
 net/hsr/hsr_netlink.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

net/hsr/hsr_netlink.c +6 -1

index db0b0af7a692..f0ca23da3ab9 100644
@@ -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");

Signature · 1 line

2.53.0