On Wed 2022-02-16 11:39:31, Joe Lawrence wrote:
> From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> klp-convert relies on libelf and on a list implementation. Add files
> scripts/livepatch/elf.c and scripts/livepatch/elf.h, which are a libelf
> interfacing layer and scripts/livepatch/list.h, which is a list
> implementation.
>
> --- /dev/null
> +++ b/scripts/livepatch/elf.c
> +static int update_shstrtab(struct elf *elf)
> +{
> + struct section *shstrtab, *sec;
> + size_t orig_size, new_size = 0, offset, len;
> + char *buf;
> +
> + shstrtab = find_section_by_name(elf, ".shstrtab");
> + if (!shstrtab) {
> + WARN("can't find .shstrtab");
> + return -1;
> + }
> +
> + orig_size = new_size = shstrtab->size;
> +
> + list_for_each_entry(sec, &elf->sections, list) {
> + if (sec->sh.sh_name != -1)
> + continue;
> + new_size += strlen(sec->name) + 1;
> + }
> +
> + if (new_size == orig_size)
> + return 0;
> +
> + buf = malloc(new_size);
> + if (!buf) {
> + WARN("malloc failed");
> + return -1;
> + }
> + memcpy(buf, (void *)shstrtab->data, orig_size);
> +
> + offset = orig_size;
> + list_for_each_entry(sec, &elf->sections, list) {
> + if (sec->sh.sh_name != -1)
> + continue;
> + sec->sh.sh_name = offset;
> + len = strlen(sec->name) + 1;
> + memcpy(buf + offset, sec->name, len);
> + offset += len;
> + }
> +
> + shstrtab->elf_data->d_buf = shstrtab->data = buf;
> + shstrtab->elf_data->d_size = shstrtab->size = new_size;
> + shstrtab->sh.sh_size = new_size;
All the update_*() functions have the same pattern. They replace
the original buffer with a bigger one when needed. And the pointer
to the original buffer gets lost.
I guess that the original buffer could not be freed because
it is part of a bigger allocated blob. Or it might even be
a file mapped to memory.
It looks like a memory leak. We could probably ignore it.
But there is another related danger, see below.
> + return 1;
> +}
> +
[...]
> +int elf_write_file(struct elf *elf, const char *file)
> +{
> + int ret_shstrtab;
> + int ret_strtab;
> + int ret_symtab;
> + int ret_relas;
We do not free the bigger buffers when something goes wrong.
Also this is not that important. But it is easy to fix:
We might do:
int ret_shstrtab = 0;
int ret_strtab = 0;
int ret_symtab = 0;
int ret_relas = 0;
> + int ret;
> +
> + ret_shstrtab = update_shstrtab(elf);
> + if (ret_shstrtab < 0)
> + return ret_shstrtab;
> +
> + ret_strtab = update_strtab(elf);
> + if (ret_strtab < 0)
> + return ret_strtab;
if (ret_strtab < 0) {
ret = ret_strtab;
goto out;
}
> + ret_symtab = update_symtab(elf);
> + if (ret_symtab < 0)
> + return ret_symtab;
if (ret_symtab < 0) {
ret = ret_symtab;
goto out;
}
> + ret_relas = update_relas(elf);
> + if (ret_relas < 0)
> + return ret_relas;
if (ret_relas < 0) {
ret = ret_relas;
goto out;
}
> + update_groups(elf);
> +
> + ret = write_file(elf, file);
> + if (ret)
> + return ret;
Continue even when write_file(elf, file) returns an error.
out:
> + if (ret_relas > 0)
> + free_relas(elf);
> + if (ret_symtab > 0)
> + free_symtab(elf);
> + if (ret_strtab > 0)
> + free_strtab(elf);
> + if (ret_shstrtab > 0)
> + free_shstrtab(elf);
> +
> + return ret;
Another problem is that the free_*() functions release the
bigger buffers. But they do not put back the original ones. Also
all the updated offsets and indexes point to the bigger buffers.
As a result the structures can't be made consistent any longer.
I am not sure if there is an easy way to fix it. IMHO, proper solution
is not worth a big effort. klp-convert frees everthing after writing
the elf file.
Well, we should at least make a comment above elf_write_file() about
that the structures are damaged in this way.
Finally, my main concern:
It brings a question whether the written data were consistent.
I am not familiar with the elf format. I quess that it is rather
stable. But there might still be some differences between
architectures or some new extensions that might need special handing.
I do not see any big consistency checks in the gelf_update_ehdr(),
elf_update(), or elf_end() functions that are used when writing
the changes.
But there seems to be some thorough consistency checks provided by:
readelf --enable-checks
It currently see these warnings:
$> readelf --lint lib/livepatch/test_klp_convert2.ko >/dev/null
readelf: Warning: Section '.note.GNU-stack': has a size of zero - is this intended ?
$> readelf --lint lib/livepatch/test_klp_callbacks_mod.ko >/dev/null
readelf: Warning: Section '.data': has a size of zero - is this intended ?
readelf: Warning: Section '.note.GNU-stack': has a size of zero - is this intended ?
$> readelf --lint lib/test_printf.ko >/dev/null
readelf: Warning: Section '.text': has a size of zero - is this intended ?
readelf: Warning: Section '.data': has a size of zero - is this intended ?
readelf: Warning: Section '.note.GNU-stack': has a size of zero - is this intended ?
But I see this warnings even without this patchset. I wonder if it
might really help to find problems introduced by klp-convert or
if it would be a waste of time.
Best Regards,
Petr
