Re: [PATCH v2 5/5] perf/arm-cmn: Support CMN S3 r2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The virtual channel count fields (rsp_vc_num, dat_vc_num, snp_vc_num, req_vc_num) in struct arm_cmn are defined as 2-bit bitfields. The patch adds support for CMN-S3 R2, which uses 3-bit values for these counts in the hardware registers. When extracting these 3-bit values and assigning them to the 2-bit bitfields, values of 4 or higher are silently truncated. Consequently, in arm_cmn_event_attr_is_visible(), the truncated value incorrectly evaluates as < 2 (e.g., 4 truncates to 0), falsely hiding valid PMU performance events from userspace.

Pre-existing issues:
- [High] In arm_cmn_discover(), the 16-bit field cmn->num_dns accumulates the number of child nodes across all XP nodes using MMIO values (CMN_CI_CHILD_COUNT). Since CMN_CI_CHILD_COUNT is a 16-bit field, accumulating multiple of these can overflow the 16-bit num_dns field. This wrapped value is used to calculate the size of the kernel heap allocation for the dn array (devm_kcalloc). During the second pass, the driver re-reads the un-wrapped hardware child counts and linearly writes to the dn array inside the loop, severely overflowing the undersized heap allocation.
--

--- Patch [5]: [PATCH v2 5/5] perf/arm-cmn: Support CMN S3 r2 ---
commit b19f5d8af33d67287a2dac3c0584d98dc6d08406
Author: Robin Murphy <robin.murphy@arm.com>

perf/arm-cmn: Support CMN S3 r2

This commit adds support for CMN S3 r2. It splits the internal model
abstraction, adds new filters, changes existing events, and provides
a means of working around the isolation feature.

> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 9d5833f8cff2b..b162de3d9d169 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c

[ ... ]

> @@ -2431,21 +2538,33 @@ static int arm_cmn_discover(struct arm_cmn *cmn, unsigned int rgn_offset)

[ ... ]

>  	reg = readq_relaxed(cfg_region + CMN_CFGM_INFO_GLOBAL_1);
> -	cmn->snp_vc_num = FIELD_GET(CMN_INFO_SNP_VC_NUM, reg);
> -	cmn->req_vc_num = FIELD_GET(CMN_INFO_REQ_VC_NUM, reg);
> +	if (model < CMNS3R2) {
> +		cmn->snp_vc_num = FIELD_GET(CMN_INFO_SNP_VC_NUM, reg);
> +		cmn->req_vc_num = FIELD_GET(CMN_INFO_REQ_VC_NUM, reg);
> +	} else {
> +		cmn->rsp_vc_num = FIELD_GET(CMN_S3_R2_RSP_VC_NUM, reg);
> +		cmn->dat_vc_num = FIELD_GET(CMN_S3_R2_DAT_VC_NUM, reg);
> +		cmn->snp_vc_num = FIELD_GET(CMN_S3_R2_SNP_VC_NUM, reg);
> +		cmn->req_vc_num = FIELD_GET(CMN_S3_R2_REQ_VC_NUM, reg);
> +	}

[Severity: Medium]
Does assigning the 3-bit CMN_S3_R2_RSP_VC_NUM value into the 2-bit
rsp_vc_num bitfield in struct arm_cmn cause silent truncation?

Looking at struct arm_cmn, the virtual channel fields are defined as 2 bits:

struct arm_cmn {
    ...
    unsigned int rsp_vc_num : 2;
    unsigned int dat_vc_num : 2;
    unsigned int snp_vc_num : 2;
    unsigned int req_vc_num : 2;
    ...
};

If a hardware value of 4 or 5 is truncated to 0 or 1 here, could it
incorrectly evaluate as < 2 in arm_cmn_event_attr_is_visible() and falsely
hide valid PMU events from userspace?

[ ... ]

>  	reg = readq_relaxed(cfg_region + CMN_CHILD_INFO);
>  	child_count = FIELD_GET(CMN_CI_CHILD_COUNT, reg);
> 
> 	cmn->num_xps = child_count;
> 	cmn->num_dns = cmn->num_xps;
> 
> 	/* Pass 1: visit the XPs, enumerate their children */
> 	cfg_region += child_poff;
> 	for (i = 0; i < cmn->num_xps; i++) {
> 		reg = readq_relaxed(cfg_region + i * 8);
> 		xp_region = cmn->base + (reg & CMN_CHILD_NODE_ADDR);
> 
> 		reg = readq_relaxed(xp_region + CMN_CHILD_INFO);
> 		cmn->num_dns += FIELD_GET(CMN_CI_CHILD_COUNT, reg);
> 	}

[Severity: High]
This is a pre-existing issue, but can accumulating 16-bit hardware counts
here overflow the 16-bit cmn->num_dns field?

If multiple XP nodes return large values for CMN_CI_CHILD_COUNT, could
cmn->num_dns wrap around, resulting in an undersized devm_kcalloc()
allocation for the dn array?

Later in the second pass, the un-wrapped counts control the loop bounds,
which seems like it could lead to linear out-of-bounds writes into the
undersized array.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785252273.git.robin.murphy@arm.com?part=5