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

Fernando Fernandez Mancera
2026-04-01T14:47:07Z

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