live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  parent reply	other threads:[~2022-04-14 15:59 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).