Re: [PATCH v8 bpf-next 02/10] libbpf: Support kind layout section handling in BTF

[Alan Maguire <alan.maguire@oracle.com>]

On 15/12/2025 09:38, bot+bpf-ci@kernel.org wrote:
>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>> index b136572e889a..8835aee6ee84 100644
>> --- a/tools/lib/bpf/btf.c
>> +++ b/tools/lib/bpf/btf.c
> 
> [ ... ]
> 
>> @@ -214,7 +232,7 @@ static int btf_add_type_idx_entry(struct btf *btf, __u32 type_off)
>>  	return 0;
>>  }
>>
>> -static void btf_bswap_hdr(struct btf_header *h)
>> +static void btf_bswap_hdr(struct btf_header *h, __u32 hdr_len)
>>  {
>>  	h->magic = bswap_16(h->magic);
>>  	h->hdr_len = bswap_32(h->hdr_len);
>> @@ -222,54 +240,87 @@ static void btf_bswap_hdr(struct btf_header *h)
>>  	h->type_len = bswap_32(h->type_len);
>>  	h->str_off = bswap_32(h->str_off);
>>  	h->str_len = bswap_32(h->str_len);
>> +	/* May be operating on raw data with hdr_len that does not include below fields */
>> +	if (hdr_len >= sizeof(struct btf_header)) {
>> +		h->kind_layout_off = bswap_32(h->kind_layout_off);
>> +		h->kind_layout_len = bswap_32(h->kind_layout_len);
>> +	}
>>  }
>>
>>  static int btf_parse_hdr(struct btf *btf)
>>  {
>> -	struct btf_header *hdr = btf->hdr;
>> +	struct btf_header *hdr = btf->raw_data;
>> +	__u32 hdr_len = hdr->hdr_len;
>>  	__u32 meta_left;
>>
>> -	if (btf->raw_size < sizeof(struct btf_header)) {
>> +	if (btf->raw_size < offsetofend(struct btf_header, str_len)) {
>>  		pr_debug("BTF header not found\n");
>>  		return -EINVAL;
>>  	}
>>
>>  	if (hdr->magic == bswap_16(BTF_MAGIC)) {
>>  		btf->swapped_endian = true;
>> -		if (bswap_32(hdr->hdr_len) != sizeof(struct btf_header)) {
>> +		hdr_len = bswap_32(hdr->hdr_len);
>> +		if (hdr_len < offsetofend(struct btf_header, str_len)) {
>>  			pr_warn("Can't load BTF with non-native endianness due to unsupported header length %u\n",
>> -				bswap_32(hdr->hdr_len));
>> +				hdr_len);
>>  			return -ENOTSUP;
>>  		}
>> -		btf_bswap_hdr(hdr);
>> +		btf_bswap_hdr(hdr, hdr_len);
>                        ^^^
> 
> Does this result in hdr->hdr_len being double-swapped? In the swapped
> endianness path, hdr_len is set to bswap_32(hdr->hdr_len) and then
> btf_bswap_hdr() is called, which does h->hdr_len = bswap_32(h->hdr_len)
> again. This means hdr->hdr_len in the structure ends up with its original
> (incorrect) endianness while the local hdr_len variable has the correct
> swapped value.
> 
> Later code then copies the incorrectly-swapped header into btf->hdr:
>

I don't think this is right. We do

	hdr_len = bswap_32(hdr->hdr_len);

and this will _return_ a bswapped value, but does not bswap hdr->hdr_len itself.
When we later bswap the header via btf_bswap_hdr(), we end up in a state where
both hdr_len and hdr->hdr_len will have the same (swapped once) value. If we
had 

	hdr->hdr_len = bswap_32(hdr->hdr_len);

...the concern would be valid. So AI wasn't far off here.
 
>>  	}
>>
>> -	if (btf->raw_size < hdr->hdr_len) {
>> +	if (btf->raw_size < hdr_len) {
>>  		pr_debug("BTF header len %u larger than data size %u\n",
>> -			 hdr->hdr_len, btf->raw_size);
>> +			 hdr_len, btf->raw_size);
>>  		return -EINVAL;
>>  	}
> 
> [ ... ]
> 
>> +	/* At this point, we have basic header information, so allocate btf->hdr */
>> +	btf->hdr = calloc(1, sizeof(struct btf_header));
>> +	if (!btf->hdr) {
>> +		pr_debug("BTF header allocation failed\n");
>> +		return -ENOMEM;
>> +	}
>> +	memcpy(btf->hdr, hdr, min((size_t)hdr_len, sizeof(struct btf_header)));
>                            ^^^
> 
> The memcpy copies from hdr, which contains the double-swapped (therefore
> incorrect) hdr_len field. So btf->hdr->hdr_len will have the wrong
> endianness when btf_parse_hdr() returns. This affects subsequent pointer
> arithmetic like:
> 
>     btf->types_data = btf->raw_data + btf->hdr->hdr_len + btf->hdr->type_off
> 
> causing types_data to point to the wrong location when loading BTF with
> non-native endianness.
>

AI has followed the chain of reasoning based on the non-existent byte-swap issue
above, so this isn't a concern either I believe.