Re: [PATCH net] net/802/mrp: fix vector attribute event count handling

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net/802/mrp: fix vector attribute event count handling

This commit fixes an issue in the MRP vector attribute parser where valen
is not decremented after processing the third event in a byte, preventing
spurious events from being applied when valen is a multiple of three.

> diff --git a/net/802/mrp.c b/net/802/mrp.c
> index ff0e80574e6b7..40cb94182167a 100644
> --- a/net/802/mrp.c
> +++ b/net/802/mrp.c

[Severity: High]
This is a pre-existing issue, but does the parser unconditionally consume
the FirstValue even when valen is 0?

According to IEEE 802.1ak, a VectorAttribute that encodes only a
LeaveAllEvent has a NumberOfValues (valen) of 0, and the FirstValue and
Vector fields are absent.

Looking at the code just before the loop, it unconditionally copies attrlen
bytes into attrvalue and advances the offset:

mrp_pdu_parse_vecattr() {
    ...
    valen = be16_to_cpu(...);
    ...
    if (skb_copy_bits(skb, *offset, mrp_cb(skb)->attrvalue,
                      mrp_cb(skb)->mh->attrlen) < 0)
        return -1;
    *offset += mrp_cb(skb)->mh->attrlen;

    while (valen > 0) {
        ...
    }
}

If a valid valen == 0 packet is processed, won't this improperly consume
bytes belonging to the next PDU element and corrupt the packet offset?

> @@ -753,6 +753,7 @@ static int mrp_pdu_parse_vecattr(struct mrp_applicant *app,
>  		vaevents %= __MRP_VECATTR_EVENT_MAX;
>  		vaevent = vaevents;
>  		mrp_pdu_parse_vecattr_event(app, skb, vaevent);
> +		valen--;

[Severity: High]
This isn't a bug introduced by this patch, but does this loop miss an
attribute value increment when crossing byte boundaries?

When valen > 3, the loop wraps around to process the next byte of events.
However, we only call mrp_attrvalue_inc() before the second and third events
inside the loop.

For the first event of the next byte (the 4th event overall), it seems we
use the exact same attribute value as the 3rd event rather than incrementing
it:

mrp_pdu_parse_vecattr() {
    ...
    while (valen > 0) {
        // ... read vaevents ...
        
        // 1st event uses current attrvalue
        mrp_pdu_parse_vecattr_event(app, skb, vaevent);
        ...
        // 2nd event increments
        mrp_attrvalue_inc(...);
        ...
        // 3rd event increments
        mrp_attrvalue_inc(...);
        ...
    }
}

Should there be an mrp_attrvalue_inc() at the end of the loop before
processing the next byte, or at the start of the loop for subsequent
iterations?

>  	}
>  	return 0;
>  }