Re: [PATCH 01/19] perf/arm-cmn: Fix event validation

[Mark Rutland <mark.rutland@arm.com>]

Hi Robin,

On Wed, Aug 13, 2025 at 06:00:53PM +0100, Robin Murphy wrote:
> In the hypothetical case where a CMN event is opened with a software
> group leader that already has some other hardware sibling, currently
> arm_cmn_val_add_event() could try to interpret the other event's data
> as an arm_cmn_hw_event, which is not great since we dereference a
> pointer from there... Thankfully the way to be more robust is to be
> less clever - stop trying to special-case software events and simply
> skip any event that isn't for our PMU.

I think this is missing some important context w.r.t. how the core perf
code behaves (and hence why this change doesn't cause other problems).
I'd suggest that we give the first few patches a common preamble:

| When opening a new perf event, the core perf code calls
| pmu::event_init() before checking whether the new event would cause an
| event group to span multiple hardware PMUs. Considering this:
| 
| (1) Any pmu::event_init() callback needs to be robust to cases where
|     a non-software group_leader or sibling event targets a distinct
|     PMU.
| 
| (2) Any pmu::event_init() callback doesn't need to explicitly reject
|     groups that span multiple hardware PMUs, as the core code will
|     reject this later.

... and then spell out the specific issues in the driver, e.g.

| The logic in arm_cmn_validate_group() doesn't account for cases where
| a non-software sibling event targets a distinct PMU. In such cases,
| arm_cmn_val_add_event() will erroneously interpret the sibling's
| event::hw as as struct arm_cmn_hw_event, including dereferencing
| pointers from potentially user-controlled fields.
|
| Fix this by skipping any events for distinct PMUs, and leaving it to
| the core code to reject event groups that span multiple hardware PMUs.

With that context, the patch itself looks good to me.

This will need a Cc stable. I'm not sure what Fixes tag is necessary;
has this been broken since its introduction?

Mark.

> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/perf/arm-cmn.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 11fb2234b10f..f8c9be9fa6c0 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1652,7 +1652,7 @@ static void arm_cmn_val_add_event(struct arm_cmn *cmn, struct arm_cmn_val *val,
>  	enum cmn_node_type type;
>  	int i;
>  
> -	if (is_software_event(event))
> +	if (event->pmu != &cmn->pmu)
>  		return;
>  
>  	type = CMN_EVENT_TYPE(event);
> @@ -1693,9 +1693,6 @@ static int arm_cmn_validate_group(struct arm_cmn *cmn, struct perf_event *event)
>  	if (leader == event)
>  		return 0;
>  
> -	if (event->pmu != leader->pmu && !is_software_event(leader))
> -		return -EINVAL;
> -
>  	val = kzalloc(sizeof(*val), GFP_KERNEL);
>  	if (!val)
>  		return -ENOMEM;
> -- 
> 2.39.2.101.g768bb238c484.dirty
>