From: Petr Mladek <pmladek@suse.com>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-kbuild@vger.kernel.org
Subject: elf API: was: Re: [RFC PATCH v6 03/12] livepatch: Add klp-convert tool
Date: Thu, 14 Apr 2022 17:03:04 +0200 [thread overview]
Message-ID: <Ylg3qKWBpryUa/t8@alley> (raw)
In-Reply-To: <20220216163940.228309-4-joe.lawrence@redhat.com>
On Wed 2022-02-16 11:39:31, Joe Lawrence wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> 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
next prev parent reply other threads:[~2022-04-14 15:45 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-16 16:39 [RFC PATCH v6 00/12] livepatch: klp-convert tool Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 01/12] livepatch: Create and include UAPI headers Joe Lawrence
2022-04-14 8:50 ` Petr Mladek
2022-02-16 16:39 ` [RFC PATCH v6 02/12] kbuild: Support for symbols.klp creation Joe Lawrence
2022-04-14 9:35 ` Petr Mladek
2022-04-14 17:59 ` Nicolas Schier
2022-04-18 18:12 ` Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 03/12] livepatch: Add klp-convert tool Joe Lawrence
2022-02-16 16:46 ` Joe Lawrence
2022-02-16 16:56 ` Joe Lawrence
2022-04-14 15:03 ` Petr Mladek [this message]
2022-04-18 18:01 ` elf API: was: " Joe Lawrence
2023-02-06 18:16 ` Marcos Paulo de Souza
2022-02-16 16:39 ` [RFC PATCH v6 04/12] livepatch: Add klp-convert annotation helpers Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 05/12] modpost: Integrate klp-convert Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 06/12] livepatch: Add sample livepatch module Joe Lawrence
2023-02-07 12:52 ` Marcos Paulo de Souza
2022-02-16 16:39 ` [RFC PATCH v6 07/12] documentation: Update on livepatch elf format Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 08/12] livepatch/selftests: add klp-convert Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 09/12] livepatch/selftests: test multiple sections Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 10/12] livepatch/selftests: add __asm__ symbol renaming examples Joe Lawrence
2022-02-16 17:03 ` Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 11/12] livepatch/selftests: add data relocations test Joe Lawrence
2022-02-16 17:12 ` Joe Lawrence
2022-02-16 16:39 ` [RFC PATCH v6 12/12] livepatch/selftests: add static keys test Joe Lawrence
2022-02-16 17:17 ` [RFC PATCH v6 00/12] livepatch: klp-convert tool Joe Lawrence
2023-02-07 12:57 ` Marcos Paulo de Souza
2023-02-07 15:54 ` Joe Lawrence
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Ylg3qKWBpryUa/t8@alley \
--to=pmladek@suse.com \
--cc=joe.lawrence@redhat.com \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).