Linux Perf Users
 help / color / mirror / Atom feed
* [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field
@ 2026-08-07  9:14 James Clark
  2026-08-07  9:27 ` sashiko-bot
  2026-08-07 10:44 ` Anshuman Khandual
  0 siblings, 2 replies; 6+ messages in thread
From: James Clark @ 2026-08-07  9:14 UTC (permalink / raw)
  To: Will Deacon, Mark Rutland, Rob Herring (Arm), Anshuman Khandual,
	Leo Yan, Suzuki Poulose
  Cc: linux-arm-kernel, linux-perf-users, linux-kernel, James Clark

PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
userspace, but it's never set by the BRBE driver. Zero initialize it as
it should be according to the docs:

   * For the architectures whose raw branch records are
   * already stored in age order, the hw_idx should be 0.

It's probably too risky to remove PERF_SAMPLE_BRANCH_HW_INDEX from BRBE
now in case anyone is setting it and reading the value, but zero
initializing the whole struct also protects against the same issue with
new fields that are added in the future.

Fixes: 58074a0fce66 ("perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)")
Signed-off-by: James Clark <james.clark@linaro.org>
---
Very small fix spotted by Sashiko. It was probably always zero during
testing or never looked at.
---
 drivers/perf/arm_pmuv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
index 8014ff766cff..b9a8592bf112 100644
--- a/drivers/perf/arm_pmuv3.c
+++ b/drivers/perf/arm_pmuv3.c
@@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
 		struct pmu_hw_events *events_cpu;
 
 		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
-		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
+		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
 		if (!events_cpu->branch_stack)
 			return -ENOMEM;
 	}

---
base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
change-id: 20260807-james-brbe-init-hw-idx-53dec54fe266

Best regards,
--  
James Clark <james.clark@linaro.org>


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field
  2026-08-07  9:14 [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field James Clark
@ 2026-08-07  9:27 ` sashiko-bot
  2026-08-07 10:05   ` James Clark
  2026-08-07 10:44 ` Anshuman Khandual
  1 sibling, 1 reply; 6+ messages in thread
From: sashiko-bot @ 2026-08-07  9:27 UTC (permalink / raw)
  To: James Clark; +Cc: linux-perf-users

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

Pre-existing issues:
- [High] Missing initialization of `new_type` and `priv` fields in `perf_clear_branch_entry_bitfields()` allows cross-sample metadata leakage and KASLR bypass.
--

--- Patch [1]: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field ---
commit 9362aa7ec453c50e757449959540fd185a720850
Author: James Clark <james.clark@linaro.org>

perf: arm_pmuv3: Zero initialize hw_id branch stack field

This patch modifies the allocation of the BRBE branch_stack to use kzalloc
instead of kmalloc. This ensures that the hw_id field, and any future fields,
are safely zero-initialized upon allocation to comply with documentation.

> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
>  		struct pmu_hw_events *events_cpu;
>  
>  		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
> -		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
> +		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but since we are fixing uninitialized branch
stack fields on allocation, does the per-sample reuse of these entries later
leak previous metadata?

The perf_clear_branch_entry_bitfields() helper zeroes most bitfields, but
misses the new_type and priv fields:

include/linux/perf_event.h:perf_clear_branch_entry_bitfields() {
    br->mispred = 0;
    br->predicted = 0;
    ...
    br->reserved = 0;
    // new_type and priv are not cleared
}

In the ARM BRBE driver, the per-CPU branch_stack buffer entries are reused
and cleared using this helper. If a hardware branch record is source-only,
entry->priv is not explicitly set:

drivers/perf/arm_brbe.c:perf_entry_from_brbe_regset() {
    perf_clear_branch_entry_bitfields(entry);
    ...
    if (!brbe_record_is_source_only(brbinf))
        entry->priv = brbinf_get_perf_priv(brbinf);
}

If the previous sample in that slot was a user-space branch, entry->priv
retains PERF_BR_PRIV_USER. When filter_branch_privilege() later checks
entry->priv, could this allow kernel source addresses to be exposed to
unprivileged users and bypass KASLR?

>  		if (!events_cpu->branch_stack)
>  			return -ENOMEM;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-james-brbe-init-hw-idx-v1-1-009d1843960a@linaro.org?part=1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field
  2026-08-07  9:27 ` sashiko-bot
@ 2026-08-07 10:05   ` James Clark
  0 siblings, 0 replies; 6+ messages in thread
From: James Clark @ 2026-08-07 10:05 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: linux-perf-users



On 07/08/2026 10:27, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing initialization of `new_type` and `priv` fields in `perf_clear_branch_entry_bitfields()` allows cross-sample metadata leakage and KASLR bypass.
> --
> 
> --- Patch [1]: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field ---
> commit 9362aa7ec453c50e757449959540fd185a720850
> Author: James Clark <james.clark@linaro.org>
> 
> perf: arm_pmuv3: Zero initialize hw_id branch stack field
> 
> This patch modifies the allocation of the BRBE branch_stack to use kzalloc
> instead of kmalloc. This ensures that the hw_id field, and any future fields,
> are safely zero-initialized upon allocation to comply with documentation.
> 
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
>>   		struct pmu_hw_events *events_cpu;
>>   
>>   		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
>> -		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
>> +		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
> 
> [Severity: High]
> This is a pre-existing issue, but since we are fixing uninitialized branch
> stack fields on allocation, does the per-sample reuse of these entries later
> leak previous metadata?
> 
> The perf_clear_branch_entry_bitfields() helper zeroes most bitfields, but
> misses the new_type and priv fields:
> 
> include/linux/perf_event.h:perf_clear_branch_entry_bitfields() {
>      br->mispred = 0;
>      br->predicted = 0;
>      ...
>      br->reserved = 0;
>      // new_type and priv are not cleared
> }
> 
> In the ARM BRBE driver, the per-CPU branch_stack buffer entries are reused
> and cleared using this helper. If a hardware branch record is source-only,
> entry->priv is not explicitly set:
> 
> drivers/perf/arm_brbe.c:perf_entry_from_brbe_regset() {
>      perf_clear_branch_entry_bitfields(entry);
>      ...
>      if (!brbe_record_is_source_only(brbinf))
>          entry->priv = brbinf_get_perf_priv(brbinf);
> }
> 
> If the previous sample in that slot was a user-space branch, entry->priv
> retains PERF_BR_PRIV_USER. When filter_branch_privilege() later checks
> entry->priv, could this allow kernel source addresses to be exposed to
> unprivileged users and bypass KASLR?
> 
>>   		if (!events_cpu->branch_stack)
>>   			return -ENOMEM;
>>   	}
> 

Fixed by "[PATCH v6 3/3] perf/core: Clear the whole branch entry in 
perf_clear_branch_entry()" on the list.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field
  2026-08-07  9:14 [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field James Clark
  2026-08-07  9:27 ` sashiko-bot
@ 2026-08-07 10:44 ` Anshuman Khandual
  2026-08-07 12:18   ` James Clark
  1 sibling, 1 reply; 6+ messages in thread
From: Anshuman Khandual @ 2026-08-07 10:44 UTC (permalink / raw)
  To: James Clark, Will Deacon, Mark Rutland, Rob Herring (Arm),
	Leo Yan, Suzuki Poulose
  Cc: linux-arm-kernel, linux-perf-users, linux-kernel

On 07/08/26 2:44 PM, James Clark wrote:
> PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
> userspace, but it's never set by the BRBE driver. Zero initialize it as
> it should be according to the docs:
> 
>    * For the architectures whose raw branch records are
>    * already stored in age order, the hw_idx should be 0.

The in code documentation while defining perf_branch_stack.
Probably a good idea to specify the same above.

 * For the architectures whose raw branch records are
 * already stored in age order, the hw_idx should be 0.
 */
struct perf_branch_stack {
	u64				nr;
	u64				hw_idx;
	struct perf_branch_entry	entries[];
};

> 
> It's probably too risky to remove PERF_SAMPLE_BRANCH_HW_INDEX from BRBE
> now in case anyone is setting it and reading the value, but zero
> initializing the whole struct also protects against the same issue with
> new fields that are added in the future.

Agreed. Because PERF_SAMPLE_BRANCH_HW_INDEX is supported in BRBE,
hw_idx pushed to the userspace should be zero if HW never updates.
This is definitely better than dropping PERF_SAMPLE_BRANCH_HW_INDEX
flag all together to avoid breaking current users (if any).
> 
> Fixes: 58074a0fce66 ("perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)")
> Signed-off-by: James Clark <james.clark@linaro.org>

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
> Very small fix spotted by Sashiko. It was probably always zero during
> testing or never looked at.
> ---
>  drivers/perf/arm_pmuv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
> index 8014ff766cff..b9a8592bf112 100644
> --- a/drivers/perf/arm_pmuv3.c
> +++ b/drivers/perf/arm_pmuv3.c
> @@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
>  		struct pmu_hw_events *events_cpu;
>  
>  		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
> -		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
> +		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
>  		if (!events_cpu->branch_stack)
>  			return -ENOMEM;
>  	}
> 
> ---
> base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
> change-id: 20260807-james-brbe-init-hw-idx-53dec54fe266
> 
> Best regards,
> --  
> James Clark <james.clark@linaro.org>
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field
  2026-08-07 10:44 ` Anshuman Khandual
@ 2026-08-07 12:18   ` James Clark
  2026-08-07 14:27     ` Will Deacon
  0 siblings, 1 reply; 6+ messages in thread
From: James Clark @ 2026-08-07 12:18 UTC (permalink / raw)
  To: Anshuman Khandual
  Cc: linux-arm-kernel, linux-perf-users, linux-kernel, Will Deacon,
	Mark Rutland, Rob Herring (Arm), Leo Yan, Suzuki Poulose



On 07/08/2026 11:44, Anshuman Khandual wrote:
> On 07/08/26 2:44 PM, James Clark wrote:
>> PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
>> userspace, but it's never set by the BRBE driver. Zero initialize it as
>> it should be according to the docs:
>>
>>     * For the architectures whose raw branch records are
>>     * already stored in age order, the hw_idx should be 0.
> 
> The in code documentation while defining perf_branch_stack.
> Probably a good idea to specify the same above.
> 
>   * For the architectures whose raw branch records are
>   * already stored in age order, the hw_idx should be 0.
>   */
> struct perf_branch_stack {
> 	u64				nr;
> 	u64				hw_idx;
> 	struct perf_branch_entry	entries[];
> };
> 

I found it easily enough. I wouldn't want to put the same comment in two 
places and risk one of them going stale. And if I take it away from one 
place and move it to the struct then it's just missing from somewhere 
else instead. So I think I'd rather leave this one.

>>
>> It's probably too risky to remove PERF_SAMPLE_BRANCH_HW_INDEX from BRBE
>> now in case anyone is setting it and reading the value, but zero
>> initializing the whole struct also protects against the same issue with
>> new fields that are added in the future.
> 
> Agreed. Because PERF_SAMPLE_BRANCH_HW_INDEX is supported in BRBE,
> hw_idx pushed to the userspace should be zero if HW never updates.
> This is definitely better than dropping PERF_SAMPLE_BRANCH_HW_INDEX
> flag all together to avoid breaking current users (if any).
>>
>> Fixes: 58074a0fce66 ("perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)")
>> Signed-off-by: James Clark <james.clark@linaro.org>
> 
> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 

Thanks

>> ---
>> Very small fix spotted by Sashiko. It was probably always zero during
>> testing or never looked at.
>> ---
>>   drivers/perf/arm_pmuv3.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/perf/arm_pmuv3.c b/drivers/perf/arm_pmuv3.c
>> index 8014ff766cff..b9a8592bf112 100644
>> --- a/drivers/perf/arm_pmuv3.c
>> +++ b/drivers/perf/arm_pmuv3.c
>> @@ -1361,7 +1361,7 @@ static int branch_records_alloc(struct arm_pmu *armpmu)
>>   		struct pmu_hw_events *events_cpu;
>>   
>>   		events_cpu = per_cpu_ptr(armpmu->hw_events, cpu);
>> -		events_cpu->branch_stack = kmalloc(size, GFP_KERNEL);
>> +		events_cpu->branch_stack = kzalloc(size, GFP_KERNEL);
>>   		if (!events_cpu->branch_stack)
>>   			return -ENOMEM;
>>   	}
>>
>> ---
>> base-commit: f9a2394a23482bfd330911e9c8295b71724feacd
>> change-id: 20260807-james-brbe-init-hw-idx-53dec54fe266
>>
>> Best regards,
>> --
>> James Clark <james.clark@linaro.org>
>>
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field
  2026-08-07 12:18   ` James Clark
@ 2026-08-07 14:27     ` Will Deacon
  0 siblings, 0 replies; 6+ messages in thread
From: Will Deacon @ 2026-08-07 14:27 UTC (permalink / raw)
  To: James Clark
  Cc: Anshuman Khandual, linux-arm-kernel, linux-perf-users,
	linux-kernel, Mark Rutland, Rob Herring (Arm), Leo Yan,
	Suzuki Poulose

On Fri, Aug 07, 2026 at 01:18:59PM +0100, James Clark wrote:
> 
> 
> On 07/08/2026 11:44, Anshuman Khandual wrote:
> > On 07/08/26 2:44 PM, James Clark wrote:
> > > PERF_SAMPLE_BRANCH_HW_INDEX is supported by BRBE so hw_id is passed to
> > > userspace, but it's never set by the BRBE driver. Zero initialize it as
> > > it should be according to the docs:
> > > 
> > >     * For the architectures whose raw branch records are
> > >     * already stored in age order, the hw_idx should be 0.
> > 
> > The in code documentation while defining perf_branch_stack.
> > Probably a good idea to specify the same above.
> > 
> >   * For the architectures whose raw branch records are
> >   * already stored in age order, the hw_idx should be 0.
> >   */
> > struct perf_branch_stack {
> > 	u64				nr;
> > 	u64				hw_idx;
> > 	struct perf_branch_entry	entries[];
> > };
> > 
> 
> I found it easily enough. I wouldn't want to put the same comment in two
> places and risk one of them going stale. And if I take it away from one
> place and move it to the struct then it's just missing from somewhere else
> instead. So I think I'd rather leave this one.

Yup, I've queued it as-is.

Cheers,

Will

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-08-07 14:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-07  9:14 [PATCH] perf: arm_pmuv3: Zero initialize hw_id branch stack field James Clark
2026-08-07  9:27 ` sashiko-bot
2026-08-07 10:05   ` James Clark
2026-08-07 10:44 ` Anshuman Khandual
2026-08-07 12:18   ` James Clark
2026-08-07 14:27     ` Will Deacon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox