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

Fernando Fernandez Mancera
2026-04-01T23:53:25Z

On 4/1/26 6:59 PM, Luka Gejak wrote:
> 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.
>

AFAIU, the TLVs must be in the right order. I don't know, it doesn't
sound very convincing to me that we are anticipating to new TLVs.
HSR/PRP isn't a very active protocol and it has few users in Kernel
probably compare to other protocols because it is used in a very
specific industry domain.

If a new revision of the protocol specs is released we can always update
our implementation.

Anyway, since Felix reviewed the initial patch let's wait for his review.

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