Re: [PATCH v2 2/6] livepatch: Add klp-convert tool

[Joe Lawrence <joe.lawrence@redhat.com>]

Hi Lukas,

As mentioned offlist, reviewing and testing this is on my TODO list, but
here are some early notes ...

On Thu, May 16, 2024 at 03:30:05PM +0200, Lukas Hruska wrote:
> Livepatches need to access external symbols which can't be handled
> by the normal relocation mechanism. It is needed for two types
> of symbols:
> 
>   + Symbols which can be local for the original livepatched function.
>     The alternative implementation in the livepatch sees them
>     as external symbols.
> 
>   + Symbols in modules which are exported via EXPORT_SYMBOL*(). They
>     must be handled special way otherwise the livepatch module would
>     depend on the livepatched one. Loading such livepatch would cause
>     loading the other module as well.
> 
> The address of these symbols can be found via kallsyms. Or they can
> be relocated using livepatch specific relocation sections as specified
> in Documentation/livepatch/module-elf-format.txt.
> 
> Currently, there is no trivial way to embed the required information as
> requested in the final livepatch elf object. klp-convert solves this
> problem by using annotations in the elf object to convert the relocation
> accordingly to the specification, enabling it to be handled by the
> livepatch loader.
> 
> Given the above, create scripts/livepatch to hold tools developed for
> livepatches and add source files for klp-convert there.
> 
> Allow to annotate such external symbols in the livepatch by a macro
> KLP_RELOC_SYMBOL(). It will create symbol with all needed
> metadata. For example:
> 
>   extern char *saved_command_line \
>                  KLP_RELOC_SYMBOL(vmlinux, vmlinux, saved_command_line, 0);

Nit: should this be KLP_RELOC_SYMBOL_POS if it including the 0 position?
(Or KLP_RELOC_SYMBOL and omit the implied 0-th position.)

> diff --git a/scripts/livepatch/elf.c b/scripts/livepatch/elf.c
> --- /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 != ~0U)
> +			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);

While reviewing klp-convert v7 [1], Alexey Dobriyan notes here,

"This code is called realloc(). :-)"

[1] https://lore.kernel.org/live-patching/4ce29654-4e1e-4680-9c25-715823ff5e02@p183/

> +static int update_strtab(struct elf *elf)
> +{
> +	struct section *strtab;
> +	struct symbol *sym;
> +	size_t orig_size, new_size = 0, offset, len;
> +	char *buf;
> +
> +	strtab = find_section_by_name(elf, ".strtab");
> +	if (!strtab) {
> +		WARN("can't find .strtab");
> +		return -1;
> +	}
> +
> +	orig_size = new_size = strtab->size;
> +
> +	list_for_each_entry(sym, &elf->symbols, list) {
> +		if (sym->sym.st_name != ~0U)
> +			continue;
> +		new_size += strlen(sym->name) + 1;
> +	}
> +
> +	if (new_size == orig_size)
> +		return 0;
> +
> +	buf = malloc(new_size);
> +	if (!buf) {
> +		WARN("malloc failed");
> +		return -1;
> +	}
> +	memcpy(buf, (void *)strtab->data, orig_size);

I think Alexey's realloc suggestion would apply here, too.

> +static int write_file(struct elf *elf, const char *file)
> +{
> +	int fd;
> +	Elf *e;
> +	GElf_Ehdr eh, ehout;
> +	Elf_Scn *scn;
> +	Elf_Data *data;
> +	GElf_Shdr sh;
> +	struct section *sec;
> +
> +	fd = creat(file, 0664);
> +	if (fd == -1) {
> +		WARN("couldn't create %s", file);
> +		return -1;
> +	}
> +
> +	e = elf_begin(fd, ELF_C_WRITE, NULL);

Alexy also found an ELF coding bug:

"elf_end() doesn't close descriptor, so there is potentially corrupted
data. There is no unlink() call if writes fail as well."

> +void elf_close(struct elf *elf)
> +{
> +	struct section *sec, *tmpsec;
> +	struct symbol *sym, *tmpsym;
> +	struct rela *rela, *tmprela;
> +
> +	list_for_each_entry_safe(sym, tmpsym, &elf->symbols, list) {
> +		list_del(&sym->list);
> +		free(sym);
> +	}
> +	list_for_each_entry_safe(sec, tmpsec, &elf->sections, list) {
> +		list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
> +			list_del(&rela->list);
> +			free(rela);
> +		}
> +		list_del(&sec->list);
> +		free(sec);
> +	}
> +	if (elf->fd > 0)
> +		close(elf->fd);

Alexy found another ELF coding bug here:

"Techically, it is "fd >= 0"."


I had coded fixes for these in a v8-devel that I never finished.  It
shouldn't be too hard to fix these up in the minimal version of the
patchset, but lmk if you'd like a patch.

That's all for now.  My plan is to try and turn off kpatch-build's
klp-relocation code and see how passing through to klp-convert fares.
That would give us a good comparison of real-world examples that need to
be handled and tested.

--
Joe