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

[Fernando Fernandez Mancera <fmancera@suse.de>]

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.

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