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

[Felix Maurer <fmaurer@redhat.com>]

On Wed, Apr 01, 2026 at 11:23:22AM +0200, 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>

As Fernando already pointed out: I did not review this.

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

As the commit message doesn't match the patch, I can only guess why this
loop is there: I assume this is because Jakub pointed you [1] to the AI
review of the patchset [2], explicitly stating that he didn't further
investigate it. The AI review touches two points, 1) a missing
pskb_may_pull() check, and 2) handling of unknown extension TLVs.

As discussed in the other email, I don't think we should do 2) here at
all. The frame format is fully specified and versioned, frames that
don't follow this are invalid.

Point 1) is actually a valid point, but the rambling of the AI why this
previously worked is wrong (it was a bug already previously). Please add
the pskb_may_pull() check in the next version.

Thanks,
   Felix


[1]: https://lore.kernel.org/netdev/20260331193758.5dd027f6@kernel.org/
[2]: https://sashiko.dev/#/patchset/20260329112313.17164-2-luka.gejak@linux.dev