Re: [PATCH v3 12/21] objtool: Refactor elf_add_data() to use a growable data buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] An integer overflow in `elf_add_data()` due to an uncontrolled `sh_addralign` value allows an out-of-bounds heap write.
- [Low] In `elf_add_data`, a failed `calloc` for the new data buffer overwrites `sec->data->d_buf` with NULL and leaks the original buffer.
--

commit 02f18d399bba6e3d751363b4e85f507e3f288e34
Author: Josh Poimboeuf <jpoimboe@kernel.org>

objtool: Refactor elf_add_data() to use a growable data buffer

Instead of calling elf_newdata() for each new piece of data with its own
separate buffer, keep it all in the same growable buffer so the
section's entire data can be accessed if needed.

> diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
> --- a/tools/objtool/elf.c
> +++ b/tools/objtool/elf.c
[ ... ]
> @@ -1409,30 +1406,55 @@ void *elf_add_data(struct elf *elf, struct section *sec, const void *data, size_
>  		return NULL;
>  	}
>  
> -	sec->data = elf_newdata(s);
>  	if (!sec->data) {
[ ... ]
> +		sec->data->d_align = sec->sh.sh_addralign;
>  	}
>  
> -	sec->data->d_buf = calloc(1, size);
> -	if (!sec->data->d_buf) {
> -		ERROR_GLIBC("calloc");
> -		return NULL;
> +	size_old = sec->data->d_size;
> +	offset = ALIGN(size_old, sec->sh.sh_addralign);
> +	size_new = offset + size;

Could this ALIGN calculation overflow if a malformed ELF file provides an
extremely large sh_addralign value?

If offset wraps around, it seems size_new might also wrap, potentially causing
roundup_pow_of_two to return a small allocation size. Could this lead to an
out-of-bounds write during the subsequent memcpy further down in this function?

> -	if (data)
> -		memcpy(sec->data->d_buf, data, size);
> +	if (!sec->data_overallocated)
> +		alloc_size_old = size_old;
> +	else
> +		alloc_size_old = max(64UL, roundup_pow_of_two(size_old ? : 1));
>  
> -	sec->data->d_size = size;
> -	sec->data->d_align = sec->sh.sh_addralign;
> +	alloc_size_new = max(64UL, roundup_pow_of_two(size_new ? : 1));
>  
> -	offset = ALIGN(sec_size(sec), sec->sh.sh_addralign);
> -	sec->sh.sh_size = offset + size;
> +	if (alloc_size_new > alloc_size_old) {
> +		void *orig_buf = sec->data->d_buf;
> +
> +		sec->data->d_buf = calloc(1, alloc_size_new);
> +		if (!sec->data->d_buf) {
> +			ERROR_GLIBC("calloc");
> +			return NULL;
> +		}

If calloc fails here, does assigning its result directly to sec->data->d_buf
clobber the valid pointer to orig_buf?

It appears orig_buf is never freed on this error path, which might leak the
original buffer if it was previously owned by objtool. Would it be safer to
keep the original buffer pointer intact on allocation failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778642120.git.jpoimboe@kernel.org?part=12