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

[Luka Gejak <luka.gejak@linux.dev>]

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