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 <[email protected]> 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/[email protected]?part=12
