Re: [PATCH v1 2/5] livepatch: Add klp-convert tool

From: Petr Mladek
Date: Fri Jan 05 2024 - 08:29:15 EST

Next message: Coiby Xu: "Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring"
Previous message: Petr Tesařík: "Re: [PATCH] net: stmmac: protect statistics updates with a spinlock"
Next in thread: Petr Mladek: "Re: [PATCH v1 2/5] livepatch: Add klp-convert tool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon 2023-11-06 17:25:10, 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

Please, remove the extra space at the end of the line.

> 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);
>
> would create symbol
>
> $>readelf -r -W <compiled livepatch module>:
> Relocation section '.rela.text' at offset 0x32e60 contains 10 entries:
> Offset Info Type Symbol's Value Symbol's Name + Addend
> [...]
> 0000000000000068 0000003c00000002 R_X86_64_PC32 0000000000000000 .klp.sym.rela.vmlinux.vmlinux.saved_command_line,0 - 4
> [...]
>
>
> Also add scripts/livepatch/klp-convert. The tool transforms symbols
> created by KLP_RELOC_SYMBOL() to object specific rela sections
> and rela entries which would later be proceed when the livepatch
> or the livepatched object is loaded.
>
> For example, klp-convert would replace the above symbols with:

s/above symbols/above symbol/

> $> readelf -r -W <livepatch_module_proceed_by_klp_convert>
> Relocation section '.klp.rela.vmlinux.text' at offset 0x5cb60 contains 1 entry:
> Offset Info Type Symbol's Value Symbol's Name + Addend
> 0000000000000068 0000003c00000002 R_X86_64_PC32 0000000000000000 .klp.sym.vmlinux.saved_command_line,0 - 4
>
> 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.
>
> Update Makefiles to correctly support the compilation of the new tool,
> update MAINTAINERS file and add a .gitignore file.
>
> ---
> MAINTAINERS | 1 +
> include/linux/livepatch.h | 19 +
> scripts/Makefile | 1 +
> scripts/livepatch/.gitignore | 1 +
> scripts/livepatch/Makefile | 5 +
> scripts/livepatch/elf.c | 817 ++++++++++++++++++++++++++++++++
> scripts/livepatch/elf.h | 73 +++

I see a similar code in

tools/objtool/elf.c
tools/objtool/include/objtool/elf.h

Both variants have been written by Josh. I wonder if we could share
one implementation. Josh?

> scripts/livepatch/klp-convert.c | 283 +++++++++++
> scripts/livepatch/klp-convert.h | 42 ++
> scripts/livepatch/list.h | 391 +++++++++++++++

And probably also the list.h

> 10 files changed, 1633 insertions(+)
> create mode 100644 scripts/livepatch/.gitignore
> create mode 100644 scripts/livepatch/Makefile
> create mode 100644 scripts/livepatch/elf.c
> create mode 100644 scripts/livepatch/elf.h
> create mode 100644 scripts/livepatch/klp-convert.c
> create mode 100644 scripts/livepatch/klp-convert.h
> create mode 100644 scripts/livepatch/list.h
>
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.c
> @@ -0,0 +1,283 @@
[...]
> +/* Converts rela symbol names */
> +static bool convert_symbol(struct symbol *s)
> +{
> + char lp_obj_name[MODULE_NAME_LEN];
> + char sym_obj_name[MODULE_NAME_LEN];
> + char sym_name[KSYM_NAME_LEN];
> + char *klp_sym_name;
> + unsigned long sym_pos;
> + int poslen;
> + unsigned int length;
> +
> + static_assert(MODULE_NAME_LEN <= 56, "Update limit in the below sscanf()");

IMHO, there should be "< 56" instead of "<= 56". The sscanf is limited by %55.

Also we should check KSYM_NAME_LEN. Similar to to check in klp_resolve_symbols()

static_assert(MODULE_NAME_LEN < 56 || KSYM_NAME_LEN != 512,
"Update limit in the below sscanf()");

> +
> + if (sscanf(s->name, KLP_SYM_RELA_PREFIX "%55[^.].%55[^.].%511[^,],%lu",
> + lp_obj_name, sym_obj_name, sym_name, &sym_pos) != 4) {
> + WARN("Invalid format of symbol (%s)\n", s->name);
> + return false;
> + }
> +
> + poslen = calc_digits(sym_pos);
> +
> + length = strlen(KLP_SYM_PREFIX) + strlen(sym_obj_name)
> + + strlen(sym_name) + sizeof(poslen) + 3;
> +
> + klp_sym_name = calloc(1, length);
> + if (!klp_sym_name) {
> + WARN("Memory allocation failed (%s%s.%s,%lu)\n", KLP_SYM_PREFIX,
> + sym_obj_name, sym_name, sym_pos);
> + return false;
> + }
> +
> + if (safe_snprintf(klp_sym_name, length, KLP_SYM_PREFIX "%s.%s,%lu",
> + sym_obj_name, sym_name, sym_pos)) {
> +
> + WARN("Length error (%s%s.%s,%lu)", KLP_SYM_PREFIX,
> + sym_obj_name, sym_name, sym_pos);
> + free(klp_sym_name);
> + return false;
> + }
> +
> + s->name = klp_sym_name;
> + s->sec = NULL;
> + s->sym.st_name = -1;
> + s->sym.st_shndx = SHN_LIVEPATCH;
> +
> + return true;
> +}
> +
> diff --git a/scripts/livepatch/klp-convert.h b/scripts/livepatch/klp-convert.h
> new file mode 100644
> index 000000000000..34842c50c711
> --- /dev/null
> +++ b/scripts/livepatch/klp-convert.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2016 Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> + * Copyright (C) 2017 Joao Moreira <jmoreira@xxxxxxx>
> + *
> + */
> +
> +#define SHN_LIVEPATCH 0xff20
> +#define SHF_RELA_LIVEPATCH 0x00100000
> +#define MODULE_NAME_LEN (64 - sizeof(GElf_Addr))
> +#define WARN(format, ...) \
> + fprintf(stderr, "klp-convert: " format "\n", ##__VA_ARGS__)
> +
> +struct sympos {
> + char *symbol_name;
> + char *object_name;
> + char *loading_obj_name;
> + int pos;
> +};

It seems that this structure is not longer used.

> +/*
> + * klp-convert uses macros and structures defined in the linux sources
> + * package (see include/uapi/linux/livepatch.h). To prevent the
> + * dependency when building locally, they are defined below. Also notice
> + * that these should match the definitions from the targeted kernel.
> + */
> +
> +#define KLP_RELA_PREFIX ".klp.rela."
> +#define KLP_SYM_RELA_PREFIX ".klp.sym.rela."
> +#define KLP_SYM_PREFIX ".klp.sym."
> +
> +#ifndef __packed
> +#define __packed __attribute__((packed))
> +#endif
> +
> +struct klp_module_reloc {
> + union {
> + void *sym;
> + uint64_t sym64; /* Force 64-bit width */
> + };
> + uint32_t sympos;
> +} __packed;

And this one as well.

I do not see any other obvious problem. And it seems to work
at least for the later added sample module.

Best Regards,
Petr

Next message: Coiby Xu: "Re: Re: [PATCH] integrity: don't throw an error immediately when failed to add a cert to the .machine keyring"
Previous message: Petr Tesařík: "Re: [PATCH] net: stmmac: protect statistics updates with a spinlock"
Next in thread: Petr Mladek: "Re: [PATCH v1 2/5] livepatch: Add klp-convert tool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]