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

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

[Yizhou Zhao]

In mrp_pdu_parse_vecattr(), vector attribute events are encoded three
per byte and valen tracks the number of events left to process.

The parser decrements valen after processing the first and second events
from each event byte, but not after processing the third one. When valen
is exactly a multiple of three, the loop continues after the last valid
event and consumes the next byte as a new event byte.

This can make the parser apply a spurious event to the current vector
attribute and update the MRP applicant state with an event that was not
part of the VectorAttribute. For example, a VectorAttribute with valen=3
and events JOIN_IN, JOIN_IN, LV can be followed by another byte whose
first encoded event is then incorrectly applied to the third attribute.

Decrement valen after processing the third event as well.

Fixes: febf018d2234 ("net/802: Implement Multiple Registration Protocol (MRP)")
Reported-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
Reported-by: Yuxiang Yang <yangyx22@mails.tsinghua.edu.cn>
Reported-by: Ao Wang <wangao@seu.edu.cn>
Reported-by: Xuewei Feng <fengxw06@126.com>
Reported-by: Qi Li <qli01@tsinghua.edu.cn>
Reported-by: Ke Xu <xuke@tsinghua.edu.cn>
Assisted-by: GLM:GLM-5.1
Signed-off-by: Yizhou Zhao <zhaoyz24@mails.tsinghua.edu.cn>
---
diff --git a/net/802/mrp.c b/net/802/mrp.c
index ff0e805..40cb941 100644
--- a/net/802/mrp.c
+++ b/net/802/mrp.c
@@ -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--;
 	}
 	return 0;
 }


--
2.43.0

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

[Simon Horman]

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