Re: [PATCH v3 next 04/13] scripts/kallsyms: make use of binary data output optional

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] scripts/kallsyms: wrong variable negated for negative offsets
  in write_src()
- [Medium] scripts/kallsyms: 9-digit buffer in write_src() clobbers sign
  on large offsets
- [Low] scripts/kallsyms: missing endian.h include for htobe32() and
  htole32()
--

commit b5cbf4ceba792715bcee0dd6d58ce67f9542a5ad
Author: David Laight <david.laight.linux@gmail.com>

scripts/kallsyms: make use of binary data output optional

This commit makes the binary output file and input ELF file optional in
scripts/kallsyms, writes the markers table in target endianness when using
binary output, and formats pc-relative offsets into a buffer for fwrite().

> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index aa3a8ec83fbb8..74db751600b4f 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
[ ... ]
> @@ -406,51 +417,105 @@ static void write_src(FILE *out_bin_file, const char *out_bin_name)
[ ... ]
> +		if (out_bin_file) {
> +			if ((i & 0xFF) == 0)
> +				markers[i >> 8] = big_endian ? htobe32(off) : htole32(off);

[Severity: Low]
Does scripts/kallsyms.c need to include <endian.h> for htobe32() and
htole32()?  Glibc pulls it in transitively through <stdlib.h>, but on
musl libc hosts neither scripts/kallsyms.c nor its included headers
declare these functions, breaking the host tool build.

[ ... ]

> @@ -458,31 +523,15 @@ static void write_src(FILE *out_bin_file, const char *out_bin_name)
[ ... ]
>  	output_label("kallsyms_offsets");
>  
>  	for (i = 0; i < table_cnt; i++) {
>  		if (pc_relative) {
> +			/* All 64 bit builds and (at least) relocatable 32 bit ones */
> +			static char line[] = "\t.long\t_text - . +123456789";
> +			char *end = line + sizeof(line) - 1;
>  			long long offset = table[i]->addr - _text;
> +			int off32;
>  
>  			if (offset < INT_MIN || offset > INT_MAX) {
>  				errx(EXIT_FAILURE, "kallsyms failure: "
> @@ -490,12 +539,28 @@ static void write_src(FILE *out_bin_file, const char *out_bin_name)
>  					table[i]->addr);
>  				exit(EXIT_FAILURE);
>  			}
> -			printf("\t.long\t_text - . + (%d)\t/* %s */\n",
> -			       (int)offset, table[i]->sym);
> +
> +			/* Build the line so the the faster fwrite() can be used */
> +			*end-- = out_bin_file ? '\n' : '\t';
> +			memset(end - 8, ' ', 8);
> +			off32 = offset;
> +			if (off32 < 0) {
> +				end[-9] = '-';
> +				offset = -off32;
> +			} else {
> +				end[-9] = '+';
> +			}

[Severity: Medium]
Should this negate off32 (or an unsigned magnitude) instead of assigning
-off32 back to offset?  Because offset is not used after this point,
off32 stays negative when a symbol lies below _text, so off32 % 10 in the
digit loop below yields negative remainders and writes non-digit ASCII
characters into the assembly output (and -off32 overflows if off32 is
INT_MIN).

> +			do
> +				*end-- = '0' + off32 % 10;
> +			while (off32 /= 10);

[Severity: Medium]
Can a 10-digit offset in [INT_MIN, INT_MAX] overwrite the '+' or '-' sign
in line[]?  The template only reserves 9 digit slots after end[-9], so any
symbol at least 1,000,000,000 bytes from _text runs 10 iterations of the
loop and clobbers end[-9], emitting invalid assembly syntax.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111115911.1142000-1-david.laight.linux@gmail.com?part=4