Re: [RFC PATCH v1 1/8] perf event: Factor build_id out into its own top-level struct

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional zeroing of build ID fields clobbers valid fallback identifiers (`maj`, `min`, `ino`), breaking symbol resolution.
- [High] Typo in bounds check writes to the wrong union member, corrupting `pgoff` and failing to clamp the build ID size.
- [Low] Modifying a tooling UAPI header without syncing the main kernel UAPI header breaks build checks.
--

--- Patch [1]: [RFC PATCH v1 1/8] perf event: Factor build_id out into its own top-level struct ---
commit 52b0cf61920b265e87e7e98a009860673a5711f3
Author: Ian Rogers <[email protected]>

perf event: Factor build_id out into its own top-level struct

This commit factors out the inline build_id struct into its own type,
struct perf_build_id, and updates various contexts to use this new
consistent type.

> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index fd10aa8d697f2..2479a38883d00 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
> @@ -1058,6 +1058,18 @@ enum perf_event_type {
>  	 */
>  	PERF_RECORD_SAMPLE			= 9,
>  

[Severity: Low]
Does modifying this tooling UAPI header without also updating the main kernel
UAPI header in include/uapi/linux/perf_event.h break the check-headers.sh
build scripts?

> +	/*
> +	 * Build IDs may be present in a number of events. They have a
> +	 * consistent encoding of:
> +	 *
> +	 * struct build_id {
> +	 *	u8		size;
> +	 *	u8		__reserved_1;
> +	 *	u16		__reserved_2;
> +	 *	u8		data[20];
> +	 * };
> +	 */

[ ... ]

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index 75a32ae8ef623..f7dedfb6bab87 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
[ ... ]
> @@ -478,16 +478,20 @@ static void perf_record_mmap2__read_build_id(struct perf_record_mmap2 *event,
>  	nsinfo__put(nsi);
>  
>  out:
> +	event->build_id.__reserved_1 = 0;
> +	event->build_id.__reserved_2 = 0;

[Severity: High]
Since struct perf_build_id is part of an anonymous union that overlaps with
the maj, min, and ino fields in struct perf_record_mmap2, do these
unconditional assignments corrupt the previously parsed fallback metadata
for binaries lacking a build ID?

>  	if (rc == 0) {
> -		memcpy(event->build_id, bid.data, sizeof(bid.data));
> -		event->build_id_size = (u8) bid.size;
> +		memcpy(event->build_id.data, bid.data, sizeof(bid.data));
> +		event->build_id.size = (u8) bid.size;
>  		event->header.misc |= PERF_RECORD_MISC_MMAP_BUILD_ID;
> -		event->__reserved_1 = 0;
> -		event->__reserved_2 = 0;
>  
>  		if (dso && !dso__has_build_id(dso))
>  			dso__set_build_id(dso, &bid);
>  	} else {
> +		memset(&event->build_id, 0, sizeof(event->build_id));

[Severity: High]
Can this memset destroy the valid maj, min, and ino identifiers when
perf_record_mmap2__read_build_id() fails to read a build ID?

> +		event->build_id.size = 0;
> +		event->header.misc &= ~PERF_RECORD_MISC_MMAP_BUILD_ID;
> +
>  		if (event->filename[0] == '/') {
>  			pr_debug2("Failed to read build ID for %s\n",
>  				  event->filename);

[ ... ]

> @@ -2486,10 +2492,10 @@ int perf_event__synthesize_mmap2_build_id(const struct perf_tool *tool,
>  	ev.mmap2.len = len;
>  	ev.mmap2.pgoff = pgoff;
>  
> -	ev.mmap2.build_id_size = bid->size;
> -	if (ev.mmap2.build_id_size > sizeof(ev.mmap2.build_id))
> -		ev.mmap2.build_id_size = sizeof(ev.mmap2.build_id);
> -	memcpy(ev.mmap2.build_id, bid->data, ev.mmap2.build_id_size);
> +	ev.mmap2.build_id.size = bid->size;
> +	if (ev.mmap2.build_id.size > sizeof(ev.mmap2.build_id.data))
> +		ev.build_id.size = sizeof(ev.mmap2.build_id.data);

[Severity: High]
Is it possible this writes to ev.build_id.size instead of
ev.mmap2.build_id.size, accidentally modifying the pgoff field of
struct perf_record_mmap2 while leaving ev.mmap2.build_id.size unclamped
for the memcpy below?

> +	memcpy(ev.mmap2.build_id.data, bid->data, ev.mmap2.build_id.size);
>  
>  	ev.mmap2.prot = prot;
>  	ev.mmap2.flags = flags;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1