Re: [RFC 1/3] kbuild: modules: Add arm64 BTI fixup utility

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] build: missing elf_flagdata() causes relocation updates to be silently discarded
- [Medium] build: POSIX exit code truncation wraps unfixed BTI counts of exactly 256 to success
- [Medium] scripts: module-bti-check conflates fatal errors with violation counts
--

commit f30f3429bd0a90f5e903b1b7fe22a7c8b76f0249
Author: Jeremy Linton <jeremy.linton@arm.com>

kbuild: modules: Add arm64 BTI fixup utility

This commit introduces a new host build tool, module-bti-check, to verify
and fix up arm64 cross-section module calls that lack BTI landing pads.
It accomplishes this by scanning R_AARCH64_CALL26 relocations and placing
veneers in the target section where necessary.

> diff --git a/scripts/module-bti-check.c b/scripts/module-bti-check.c
> new file mode 100644
> index 0000000000000..c8bef0ed90bb7
> --- /dev/null
> +++ b/scripts/module-bti-check.c

[ ... ]

>  			if (fix) {
>  				ret = get_veneer(obj, veneers, &target, &veneer);
>  				if (ret)
>  					goto out;
>  
>  				rela.r_info = GELF_R_INFO(veneer->secsym, R_AARCH64_CALL26);
>  				rela.r_addend = veneer->value;
>  				if (!gelf_update_rela(data, j, &rela))
>  					return elf_error(obj);

[Severity: High]
Does this miss setting the libelf dirty flag on the data descriptor?

When gelf_update_rela() is called in scan_relocations(), it updates the
in-memory buffer, but libelf requires elf_flagdata(data, ELF_C_SET, 
ELF_F_DIRTY) to actually write the modifications back to the file during
elf_update(). 

Without the dirty flag, won't the new landing pad veneers be silently
discarded when the updated .rela section is dropped during the file write?

>  
>  				obj->changed = true;
>  				continue;
>  			}

[ ... ]

>  			fprintf(stderr,
>  				"%s: cross-section call from %s%s%s+0x%llx to %s (%s+0x%llx) lacks a BTI landing pad\n",
>  				obj->name, source ?: "", source ? " " : "",
>  				section_name(obj, shdr.sh_info),
>  				(unsigned long long)rela.r_offset, target.name,
>  				section_name(obj, target.section_index),
>  				(unsigned long long)target.value);
>  
>  			unfixed++;
>  		}
>  	}
>  
>  	ret = unfixed;
>  out:
>  	return ret;

[Severity: Medium]
Is there a risk of exit code truncation here? scan_relocations() returns
the raw count of unfixed occurrences, which is eventually propagated up
to main() and used as the process exit code.

Since POSIX exit codes are truncated to 8 bits, what happens if the tool
encounters exactly 256 (or 512, etc.) unfixed occurrences? It appears 
returning 256 would wrap to 0, which might trick the build system into 
treating it as a successful execution despite missing BTI pads.

[ ... ]

>  	/* Must happen after elf end, to assure correctness */
>  	while (veneers) {
>  		struct veneer *next = veneers->next;
>  
>  		free(veneers);
>  		veneers = next;
>  	}
>  
>  	if (fd >= 0)
>  		close(fd);
>  
>  	if (ret < 0) {
>  		if (ret != -ELIBBAD)
>  			fprintf(stderr, "%s: %s\n", name, strerror(-ret));
>  		return EXIT_FAILURE;
>  	}
>  
>  	return ret;
>  }

[Severity: Medium]
Could this error handling conflate fatal OS or libelf errors with violation
counts?

In process_file(), negative error codes like -ENOMEM are converted to
EXIT_FAILURE (which is 1). Because this overlaps with the value returned
when there is exactly 1 unfixed violation, main() cannot differentiate
between a tool failure and a BTI warning.

[ ... ]

>  	if (elf_version(EV_CURRENT) == EV_NONE) {
>  		fprintf(stderr, "libelf initialization failed: %s\n", elf_errmsg(-1));
>  		goto out;
>  	}
>  
>  	for (i = optind; i < argc; i++) {
>  		int status = process_file(argv[i], fix);
>  
>  		if (status && !warn) {
>  			ret = status;
>  			goto out;
>  		}
>  	}

When the --warn flag is active, it ignores all non-zero status codes. Won't
this silently mask build environment failures, returning success even if 
the file couldn't be opened or processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006221815.2823252-1-jeremy.linton@arm.com?part=1