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

Luka Gejak
2026-04-01T16:59:50Z

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