Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Thu, 14 Apr 2022 17:03:04 +0200
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
Message-ID: <Ylg3qKWBpryUa/t8@alley>
References: <20220216163940.228309-1-joe.lawrence@redhat.com>
 <20220216163940.228309-4-joe.lawrence@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220216163940.228309-4-joe.lawrence@redhat.com>
Precedence: bulk

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