| Message ID | 20230306140824.3858543-1-joe.lawrence@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | livepatch: klp-convert tool | expand |
On Mon, Mar 06, 2023 at 09:08:14AM -0500, Joe Lawrence wrote: > Summary > ------- > > Livepatches may use symbols which are not contained in its own scope, > and, because of that, may end up compiled with relocations that will > only be resolved during module load. Yet, when the referenced symbols > are not exported, solving this relocation requires information on the > object that holds the symbol (either vmlinux or modules) and its > position inside the object, as an object may contain multiple symbols > with the same name. Providing such information must be done accordingly > to what is 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 in two different forms: (i) by relying on a symbol map, which is > built during kernel compilation, to automatically infer the relocation > targeted symbol, and, when such inference is not possible (ii) 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, add support for symbol mapping in the form of a > symbols.klp file; add klp-convert tool; integrate klp-convert tool into > kbuild; make livepatch modules discernible during kernel compilation > pipeline; add data-structure and macros to enable users to annotate > livepatch source code; make modpost stage compatible with livepatches; > update livepatch-sample and update documentation. > > The patch was tested under three use-cases: > > use-case 1: There is a relocation in the lp that can be automatically > resolved by klp-convert. For example. see the saved_command_line > variable in lib/livepatch/test_klp_convert2.c. > > use-case 2: There is a relocation in the lp that cannot be automatically > resolved, as the name of the respective symbol appears in multiple > objects. The livepatch contains an annotation to enable a correct > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections > in lib/livepatch/test_klp_convert{1,2}.c. > > use-case 3: There is a relocation in the lp that cannot be automatically > resolved similarly as 2, but no annotation was provided in the > livepatch, triggering an error during compilation. Reproducible by > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in > lib/livepatch/test_klp_convert{1,2}.c. > > Selftests have been added to exercise these klp-convert use-cases > through several tests. > > > Testing > ------- > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > for both default config (with added livepatch dependencies) and a larger > RHEL-9-ish config. > > Using the Intel's Linux Kernel Performance tests's make.cross, > klp-convert builds and processes livepatch .ko's for x86_64 ppc64le > ppc32 s390 arm64 arches. > > > Summary of changes in v7 > ------------------------ > > - rebase for v6.2 > - combine ("livepatch: Add klp-convert tool") with ("livepatch: Add > klp-convert annotation helpers") > - combine ("kbuild: Support for symbols.klp creation") with ("modpost: > Integrate klp-convert") to simplify Kbuild magic [Petr, Nicolas] > - klp-convert: add safe_snprintf() (-Wsign-compare) > - klp-convert: fix -Wsign-compare warnings > - klp-convert: use calloc() where appropriate > - klp-convert: copy ELF e_flags > - selftests: fix various build warnings > - klp-convert: WARN msg simplification, failed sanity checks, and sympos > comment [Marcos] > - klp-convert: fix elf_write_file() error paths [Petr] Thanks for the new version Joe. I've run the ksefltests on my x86 laptop, and it succeed as expected, so Tested-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > > Previous versions > ----------------- > > RFC: > https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ > v2: > https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ > v3: > https://lore.kernel.org/lkml/20190410155058.9437-1-joe.lawrence@redhat.com/ > v4: > https://lore.kernel.org/lkml/20190509143859.9050-1-joe.lawrence@redhat.com/ > v5: > (not posted) > https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v5-devel > v6: > https://lore.kernel.org/live-patching/20220216163940.228309-1-joe.lawrence@redhat.com/ > > > Joe Lawrence (10): > livepatch: Create and include UAPI headers > livepatch: Add klp-convert tool > kbuild/modpost: create symbols.klp and integrate klp-convert > livepatch: Add sample livepatch module > documentation: Update on livepatch elf format > livepatch/selftests: add klp-convert > livepatch/selftests: test multiple sections > livepatch/selftests: add __asm__ symbol renaming examples > livepatch/selftests: add data relocations test > livepatch/selftests: add static keys test > > .gitignore | 2 + > Documentation/dontdiff | 1 + > Documentation/livepatch/livepatch.rst | 3 + > Documentation/livepatch/module-elf-format.rst | 42 +- > MAINTAINERS | 2 + > Makefile | 16 +- > include/linux/livepatch.h | 13 + > include/uapi/linux/livepatch.h | 25 + > kernel/livepatch/core.c | 4 +- > lib/livepatch/Makefile | 12 + > lib/livepatch/test_klp_convert.h | 45 + > lib/livepatch/test_klp_convert1.c | 121 +++ > lib/livepatch/test_klp_convert2.c | 110 +++ > lib/livepatch/test_klp_convert_data.c | 190 ++++ > lib/livepatch/test_klp_convert_keys.c | 91 ++ > lib/livepatch/test_klp_convert_keys_mod.c | 52 + > lib/livepatch/test_klp_convert_mod_a.c | 31 + > lib/livepatch/test_klp_convert_mod_b.c | 19 + > lib/livepatch/test_klp_convert_mod_c.c | 36 + > lib/livepatch/test_klp_convert_sections.c | 120 +++ > samples/livepatch/Makefile | 1 + > .../livepatch/livepatch-annotated-sample.c | 93 ++ > scripts/Makefile | 1 + > scripts/Makefile.modfinal | 33 + > scripts/Makefile.modpost | 5 + > scripts/livepatch/.gitignore | 1 + > scripts/livepatch/Makefile | 5 + > scripts/livepatch/elf.c | 817 ++++++++++++++++ > scripts/livepatch/elf.h | 74 ++ > scripts/livepatch/klp-convert.c | 893 ++++++++++++++++++ > scripts/livepatch/klp-convert.h | 47 + > scripts/livepatch/list.h | 391 ++++++++ > scripts/mod/modpost.c | 28 +- > scripts/mod/modpost.h | 1 + > .../selftests/livepatch/test-livepatch.sh | 403 ++++++++ > 35 files changed, 3716 insertions(+), 12 deletions(-) > create mode 100644 include/uapi/linux/livepatch.h > create mode 100644 lib/livepatch/test_klp_convert.h > create mode 100644 lib/livepatch/test_klp_convert1.c > create mode 100644 lib/livepatch/test_klp_convert2.c > create mode 100644 lib/livepatch/test_klp_convert_data.c > create mode 100644 lib/livepatch/test_klp_convert_keys.c > create mode 100644 lib/livepatch/test_klp_convert_keys_mod.c > create mode 100644 lib/livepatch/test_klp_convert_mod_a.c > create mode 100644 lib/livepatch/test_klp_convert_mod_b.c > create mode 100644 lib/livepatch/test_klp_convert_mod_c.c > create mode 100644 lib/livepatch/test_klp_convert_sections.c > create mode 100644 samples/livepatch/livepatch-annotated-sample.c > 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 > > -- > 2.39.2 >
On Tue, Mar 14, 2023 at 05:23:56PM -0300, Marcos Paulo de Souza wrote: > On Mon, Mar 06, 2023 at 09:08:14AM -0500, Joe Lawrence wrote: > > Summary > > ------- > > > > Livepatches may use symbols which are not contained in its own scope, > > and, because of that, may end up compiled with relocations that will > > only be resolved during module load. Yet, when the referenced symbols > > are not exported, solving this relocation requires information on the > > object that holds the symbol (either vmlinux or modules) and its > > position inside the object, as an object may contain multiple symbols > > with the same name. Providing such information must be done accordingly > > to what is 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 in two different forms: (i) by relying on a symbol map, which is > > built during kernel compilation, to automatically infer the relocation > > targeted symbol, and, when such inference is not possible (ii) 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, add support for symbol mapping in the form of a > > symbols.klp file; add klp-convert tool; integrate klp-convert tool into > > kbuild; make livepatch modules discernible during kernel compilation > > pipeline; add data-structure and macros to enable users to annotate > > livepatch source code; make modpost stage compatible with livepatches; > > update livepatch-sample and update documentation. > > > > The patch was tested under three use-cases: > > > > use-case 1: There is a relocation in the lp that can be automatically > > resolved by klp-convert. For example. see the saved_command_line > > variable in lib/livepatch/test_klp_convert2.c. > > > > use-case 2: There is a relocation in the lp that cannot be automatically > > resolved, as the name of the respective symbol appears in multiple > > objects. The livepatch contains an annotation to enable a correct > > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections > > in lib/livepatch/test_klp_convert{1,2}.c. > > > > use-case 3: There is a relocation in the lp that cannot be automatically > > resolved similarly as 2, but no annotation was provided in the > > livepatch, triggering an error during compilation. Reproducible by > > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in > > lib/livepatch/test_klp_convert{1,2}.c. > > > > Selftests have been added to exercise these klp-convert use-cases > > through several tests. > > > > > > Testing > > ------- > > > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > > for both default config (with added livepatch dependencies) and a larger > > RHEL-9-ish config. > > > > Using the Intel's Linux Kernel Performance tests's make.cross, > > klp-convert builds and processes livepatch .ko's for x86_64 ppc64le > > ppc32 s390 arm64 arches. > > > > > > Summary of changes in v7 > > ------------------------ > > > > - rebase for v6.2 > > - combine ("livepatch: Add klp-convert tool") with ("livepatch: Add > > klp-convert annotation helpers") > > - combine ("kbuild: Support for symbols.klp creation") with ("modpost: > > Integrate klp-convert") to simplify Kbuild magic [Petr, Nicolas] > > - klp-convert: add safe_snprintf() (-Wsign-compare) > > - klp-convert: fix -Wsign-compare warnings > > - klp-convert: use calloc() where appropriate > > - klp-convert: copy ELF e_flags > > - selftests: fix various build warnings > > - klp-convert: WARN msg simplification, failed sanity checks, and sympos > > comment [Marcos] > > - klp-convert: fix elf_write_file() error paths [Petr] > > Thanks for the new version Joe. I've run the ksefltests on my x86 laptop, and it > succeed as expected, so > > Tested-by: Marcos Paulo de Souza <mpdesouza@suse.com> > Thanks for the testing and reviews, Marcos. The selftests are the first level of testing... we should probably tackle a real or simulated CVE fix to see how well the tooling fits larger livepatches. One complication that I can envision is symbol positioning. Currently, the klp-convert annotations are a direct mirror of the kernel's <obj,symbol,pos> tuple. It should be possible to make this a bit more user friendly for the livepatch developer if the annotations were <obj,file,symbol>, as derived from the vmlinux / module.tmp.ko symbol tables. For example, the following code: KLP_MODULE_RELOC(test_klp_convert_mod, test_klp_convert_mod_b.c) test_klp_convert_mod_relocs_b[] = { KLP_SYMPOS(homonym_string), KLP_SYMPOS(get_homonym_string), }; could generate the following relocations: Relocation section '.rela.klp.module_relocs.test_klp_convert_mod.test_klp_convert_mod_b.c' at offset 0x1dc0 contains 2 entries: Offset Info Type Symbol's Value Symbol's Name + Addend 0000000000000000 0000003f00000001 R_X86_64_64 0000000000000000 homonym_string + 0 0000000000000008 0000004900000001 R_X86_64_64 0000000000000000 get_homonym_string + 0 for which klp-convert looks up in symbols.klp: klp-convert-symbol-data.0.2 *vmlinux ... *test_klp_convert_mod -test_klp_convert_mod_a.c << added filenames to the format test_klp_get_driver_name driver_name get_homonym_string << sympos = 1 homonym_string << sympos = 1 ... -test_klp_convert_mod_b.c get_homonym_string << sympos = 2 homonym_string << sympos = 2 ... and then generates the usual klp-relocations as currently defined. (Unfortunately full pathnames are not saved in the STT_FILE symbol table entries, so there will be a few non-unique <obj,file,symbol> entries. I believe the last time this was discussed, we found that there were a relatively small number of such symbols.) Have you tried retrofitting klp-convert into any real-world livepatch? I'm curious as to your observations on the overall experience, or thoughts on the sympos annotation style noted above. Regards, -- Joe
On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: > Have you tried retrofitting klp-convert into any real-world livepatch? > I'm curious as to your observations on the overall experience, or > thoughts on the sympos annotation style noted above. On a related note, the patch creation process (of which klp-convert would be part of) needs to be documented. If I remember correctly, the proper safe usage of klp-convert requires a kernel built with -flive-patching, plus some scripting and/or manual processes. If nobody knows how to safely use it then there wouldn't be much value in merging it.
On 3/17/23 19:20, Josh Poimboeuf wrote: > On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: >> Have you tried retrofitting klp-convert into any real-world livepatch? >> I'm curious as to your observations on the overall experience, or >> thoughts on the sympos annotation style noted above. > > On a related note, the patch creation process (of which klp-convert > would be part of) needs to be documented. > > If I remember correctly, the proper safe usage of klp-convert requires a > kernel built with -flive-patching, plus some scripting and/or manual > processes. > > If nobody knows how to safely use it then there wouldn't be much value > in merging it. > I took a trip though my inbox and dug up a 2020 discussion on documenting livepatch and compiler considerations [1]. This led to the suggestion of a greater, end-to-end livepatch author guide, for which everyone agreed, but has since remained unwritten :/ If we have a miniconf @ LPC this year, maybe we can volunteer to write sections, decide on an outline, or even better, have a rough draft to review and discuss? Aside: technically klp-convert doesn't require -flive-patching, but it's probably strongly recommended in order to use it safely. And fwiw, kpatch-build could leverage the tool should it desire one day. In the meantime, if kpatch-build doesn't need/want to use it, perhaps klp-convert should have its own CONFIG option? (Or something in modinfo to key on.) [1] https://lore.kernel.org/live-patching/20200721161407.26806-1-joe.lawrence@redhat.com/
On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: > On Tue, Mar 14, 2023 at 05:23:56PM -0300, Marcos Paulo de Souza wrote: > > On Mon, Mar 06, 2023 at 09:08:14AM -0500, Joe Lawrence wrote: > > > Summary > > > ------- > > > > > > Livepatches may use symbols which are not contained in its own scope, > > > and, because of that, may end up compiled with relocations that will > > > only be resolved during module load. Yet, when the referenced symbols > > > are not exported, solving this relocation requires information on the > > > object that holds the symbol (either vmlinux or modules) and its > > > position inside the object, as an object may contain multiple symbols > > > with the same name. Providing such information must be done accordingly > > > to what is 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 in two different forms: (i) by relying on a symbol map, which is > > > built during kernel compilation, to automatically infer the relocation > > > targeted symbol, and, when such inference is not possible (ii) 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, add support for symbol mapping in the form of a > > > symbols.klp file; add klp-convert tool; integrate klp-convert tool into > > > kbuild; make livepatch modules discernible during kernel compilation > > > pipeline; add data-structure and macros to enable users to annotate > > > livepatch source code; make modpost stage compatible with livepatches; > > > update livepatch-sample and update documentation. > > > > > > The patch was tested under three use-cases: > > > > > > use-case 1: There is a relocation in the lp that can be automatically > > > resolved by klp-convert. For example. see the saved_command_line > > > variable in lib/livepatch/test_klp_convert2.c. > > > > > > use-case 2: There is a relocation in the lp that cannot be automatically > > > resolved, as the name of the respective symbol appears in multiple > > > objects. The livepatch contains an annotation to enable a correct > > > relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections > > > in lib/livepatch/test_klp_convert{1,2}.c. > > > > > > use-case 3: There is a relocation in the lp that cannot be automatically > > > resolved similarly as 2, but no annotation was provided in the > > > livepatch, triggering an error during compilation. Reproducible by > > > removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in > > > lib/livepatch/test_klp_convert{1,2}.c. > > > > > > Selftests have been added to exercise these klp-convert use-cases > > > through several tests. > > > > > > > > > Testing > > > ------- > > > > > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > > > for both default config (with added livepatch dependencies) and a larger > > > RHEL-9-ish config. > > > > > > Using the Intel's Linux Kernel Performance tests's make.cross, > > > klp-convert builds and processes livepatch .ko's for x86_64 ppc64le > > > ppc32 s390 arm64 arches. > > > > > > > > > Summary of changes in v7 > > > ------------------------ > > > > > > - rebase for v6.2 > > > - combine ("livepatch: Add klp-convert tool") with ("livepatch: Add > > > klp-convert annotation helpers") > > > - combine ("kbuild: Support for symbols.klp creation") with ("modpost: > > > Integrate klp-convert") to simplify Kbuild magic [Petr, Nicolas] > > > - klp-convert: add safe_snprintf() (-Wsign-compare) > > > - klp-convert: fix -Wsign-compare warnings > > > - klp-convert: use calloc() where appropriate > > > - klp-convert: copy ELF e_flags > > > - selftests: fix various build warnings > > > - klp-convert: WARN msg simplification, failed sanity checks, and sympos > > > comment [Marcos] > > > - klp-convert: fix elf_write_file() error paths [Petr] > > > > Thanks for the new version Joe. I've run the ksefltests on my x86 laptop, and it > > succeed as expected, so > > > > Tested-by: Marcos Paulo de Souza <mpdesouza@suse.com> > > > > Thanks for the testing and reviews, Marcos. > > The selftests are the first level of testing... we should probably > tackle a real or simulated CVE fix to see how well the tooling fits > larger livepatches. Our plan is to start testing new livepatches with klp-convert to ensure that it works as expected, removing the need of kallsyms_lookup. Let's see how it goes in the next few weeks. > > One complication that I can envision is symbol positioning. Currently, > the klp-convert annotations are a direct mirror of the kernel's > <obj,symbol,pos> tuple. It should be possible to make this a bit more > user friendly for the livepatch developer if the annotations were > <obj,file,symbol>, as derived from the vmlinux / module.tmp.ko symbol > tables. > > For example, the following code: > > KLP_MODULE_RELOC(test_klp_convert_mod, test_klp_convert_mod_b.c) test_klp_convert_mod_relocs_b[] = { > KLP_SYMPOS(homonym_string), > KLP_SYMPOS(get_homonym_string), > }; > > could generate the following relocations: > > Relocation section '.rela.klp.module_relocs.test_klp_convert_mod.test_klp_convert_mod_b.c' at offset 0x1dc0 contains 2 entries: > Offset Info Type Symbol's Value Symbol's Name + Addend > 0000000000000000 0000003f00000001 R_X86_64_64 0000000000000000 homonym_string + 0 > 0000000000000008 0000004900000001 R_X86_64_64 0000000000000000 get_homonym_string + 0 > > for which klp-convert looks up in symbols.klp: > > klp-convert-symbol-data.0.2 > *vmlinux > ... > *test_klp_convert_mod > -test_klp_convert_mod_a.c << added filenames to the format > test_klp_get_driver_name > driver_name > get_homonym_string << sympos = 1 > homonym_string << sympos = 1 > ... > -test_klp_convert_mod_b.c > get_homonym_string << sympos = 2 > homonym_string << sympos = 2 > ... > > and then generates the usual klp-relocations as currently defined. This can get verbose very quickly, but I liked the idea. The sympos is much more cryptic than specifying the file where the symbol lives on. > > (Unfortunately full pathnames are not saved in the STT_FILE symbol table > entries, so there will be a few non-unique <obj,file,symbol> entries. I > believe the last time this was discussed, we found that there were a > relatively small number of such symbols.) > > Have you tried retrofitting klp-convert into any real-world livepatch? > I'm curious as to your observations on the overall experience, or > thoughts on the sympos annotation style noted above. As I mentioned I'll start using klp-convert with new livepaches for testing purposes in the next coming weeks. I'll reply in a few weeks about the experience so far. > > Regards, > > -- Joe >
Josh Poimboeuf <jpoimboe@kernel.org> writes: > On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: >> Have you tried retrofitting klp-convert into any real-world livepatch? >> I'm curious as to your observations on the overall experience, or >> thoughts on the sympos annotation style noted above. > > On a related note, the patch creation process (of which klp-convert > would be part of) needs to be documented. > > If I remember correctly, the proper safe usage of klp-convert requires a > kernel built with -flive-patching, plus some scripting and/or manual > processes. Not always, I think: -flive-patching or IPA optimizations in general aren't a concern in the context of data symbols. From a quick glance, it seems like the selftests introduced as part of this patchset are all restricted to this usecase. > If nobody knows how to safely use it then there wouldn't be much value > in merging it. I tend to agree, but would put it a bit differently: the current implementation of klp-convert features quite some convenience logic, which, until the question of a documented livepatch preparation process has been settled, is not known yet to ever be of any use. For example, from [3/10]: "For automatic resolution of livepatch relocations, a file called symbols.klp is used. This file maps symbols within every compiled kernel object allowing the identification of symbols whose name is unique, thus relocation can be automatically inferred, or providing information that helps developers when code annotation is required for solving the matter." For the source based approach to livepatch preparation we're using internally, this is not really needed: the entity generating the source -- be it klp-ccp or the author doing it manually -- needs to examine the target objects long before link resp. klp-convert time for which symbols can be referenced from the livepatch and how (i.e. determine a potential sympos). I would expect it works similar for kpatch-build conceptually, albeit kpatch-build probably doesn't rely on any external utility like klp-convert for the .klp.* relas generation at all. So with that, I agree that merging the klp-convert patchset in its current form with those potentially unused convenience features, presumably born out of certain assumptions about a manual livepatch preparation process, indeed can be argued about, probably. However, OTOH, there's currently no means whatsoever to create those .klp.* relas (*) (**) and I would like to propose resorting to a more minimal utility doing only that single thing: to stubbornly create .klp.* relas out of certain "regular" ones using a very simple transformation rule and nothing else beyond that. The "stripped" klp-convert would have no knowledge of the symbols available in the livepatched target objects at all, i.e. there would be no symbols.klp file or alike anymore. Instead, it would simply walk through all of a livepatch object's SHN_UNDEF symbols of format ".klp.sym.<loading-obj-name>.<foo-providing-mod>.some_foo,0" somewhen at modpost time and - rename the symbol to ".klp.sym.<foo-providing-mod>.some_foo,0" -- shortening the name should always be feasible as far as strtab is concerned. - turn the symbol's SHN_UNDEF into SHN_LIVEPATCH - move any relocation (initially created by the compiler with source based lp preparation approaches) against this symbol into a separate, newly created rela section with flag SHF_RELA_LIVEPATCH set and whose name is of format .klp.rela.<loading-obj-name>.<livepatch-obj-dst-section-name>. Furthermore, the new .klp.rela section's ->sh_info needs to be made to refer to the destination section. So, the only thing which would depend on the yet unspecified details of the livepatch preparation process would be the creation of those intermediate ".klp.sym.<loading-obj-name>.<foo-providing-mod>.some_foo,0" SHN_UNDEF symbols to be processed by klp-convert. For source based livepatch preparation approaches, counting in the selftests, this can be easily controlled by means of asm("...") alias specifications at the respective declarations like in e.g. extern int foo asm("\".klp.sym.<loading-obj-name>.<foo-providing-mod>.some_foo,0\""); I imagine the first ones to benefit from having such a "stripped" klp-convert available in the kernel tree would be new upstream selftests for .klp.* rela coverage (like introduced with this here patchset already) and for those some means of creating .klp.* relas would be needed anyway. We (SUSE), and perhaps others as well, could integrate this "stripped" klp-convert into our source based, production livepatch preparation workflows right away, of course, and so we're obviously keen on having it. Such a tool providing only the bare minimum would be pretty much self-contained -- it would only need to hook into the modpost Kbuild stage one way or the other -- and we could certainly maintain it downstream out-of-tree, but that would potentially only contribute to the current fragmentation around the livepatch creation processes even more and there still wouldn't have a solution for the upstream selftests. What do you think, does it make sense to eventually have such a bare minimum klp-convert merged in-tree, independently of the ongoing discussion around the livepatch preparation processes, respectively (the lack of) documentation around it? If yes, Lukas, now on CC, is interested in this topic and would be willing to help out in any form desired: either by contributing to Joe's work here or, if deemed more feasible, to start out completely new from scratch -- dependent on your opinion on the proposed, more minimal approach as well as on Joe's plans around klp-convert. Looking forward to hearing your feedback! Thanks, Nicolai (*) We've been experimenting with building the relocation records manually by various means, e.g. with GNU as' .reloc directive as an example, but this all turned out impractical for various reasons. Most noteworthy, because the records' offsets wouldn't get adjusted properly when linking AFAIR. (**) by some other means than directly with kpatch-build
> Testing > ------- > > The patchset selftests build and execute on x86_64, s390x, and ppc64le > for both default config (with added livepatch dependencies) and a larger > RHEL-9-ish config. > > Using the Intel's Linux Kernel Performance tests's make.cross, > klp-convert builds and processes livepatch .ko's for x86_64 ppc64le > ppc32 s390 arm64 arches. > So I tested using real x86 livepatches, and it worked as expected. The test was done by taking the livepatches, removing all externalized variables (those that we need to call kallsyms_lookup), and compiling as usual and using klp-convert to create the necessary relocation entries. I'll keep testing with more livepatches for now on, and let you know if I find any issue.
On Tue, Apr 11, 2023 at 12:06:43PM +0200, Nicolai Stange wrote: > Josh Poimboeuf <jpoimboe@kernel.org> writes: > > > On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: > >> Have you tried retrofitting klp-convert into any real-world livepatch? > >> I'm curious as to your observations on the overall experience, or > >> thoughts on the sympos annotation style noted above. > > > > On a related note, the patch creation process (of which klp-convert > > would be part of) needs to be documented. > > > > If I remember correctly, the proper safe usage of klp-convert requires a > > kernel built with -flive-patching, plus some scripting and/or manual > > processes. > > Not always, I think: -flive-patching or IPA optimizations in general > aren't a concern in the context of data symbols. From a quick glance, it > seems like the selftests introduced as part of this patchset are > all restricted to this usecase. <snip> > What do you think, does it make sense to eventually have such a bare > minimum klp-convert merged in-tree, independently of the ongoing > discussion around the livepatch preparation processes, respectively (the > lack of) documentation around it? If yes, Lukas, now on CC, is > interested in this topic and would be willing to help out in any form > desired: either by contributing to Joe's work here or, if deemed more > feasible, to start out completely new from scratch -- dependent on your > opinion on the proposed, more minimal approach as well as on Joe's plans > around klp-convert. > > Looking forward to hearing your feedback! So guys, any feedback? Ping :) > > Thanks, > > Nicolai > > (*) We've been experimenting with building the relocation records > manually by various means, e.g. with GNU as' .reloc directive as an > example, but this all turned out impractical for various > reasons. Most noteworthy, because the records' offsets wouldn't get > adjusted properly when linking AFAIR. > > (**) by some other means than directly with kpatch-build > > -- > SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nürnberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > (HRB 36809, AG Nürnberg)
On 4/11/23 06:06, Nicolai Stange wrote: > Josh Poimboeuf <jpoimboe@kernel.org> writes: > >> On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: >>> Have you tried retrofitting klp-convert into any real-world livepatch? >>> I'm curious as to your observations on the overall experience, or >>> thoughts on the sympos annotation style noted above. >> >> On a related note, the patch creation process (of which klp-convert >> would be part of) needs to be documented. >> >> If I remember correctly, the proper safe usage of klp-convert requires a >> kernel built with -flive-patching, plus some scripting and/or manual >> processes. > > Not always, I think: -flive-patching or IPA optimizations in general > aren't a concern in the context of data symbols. From a quick glance, it > seems like the selftests introduced as part of this patchset are > all restricted to this usecase. > IIRC there is nothing currently stopping klp-convert from converting function symbol relocations. That may be dangerous when taking optimizations like sibling functions (and their sharing of stack) into consideration. This is about the point I stopped to turn and see what the real use cases may be. >> If nobody knows how to safely use it then there wouldn't be much value >> in merging it. > > I tend to agree, but would put it a bit differently: the current > implementation of klp-convert features quite some convenience logic, > which, until the question of a documented livepatch preparation process > has been settled, is not known yet to ever be of any use. > Good observation and perhaps something that Marcos could elaborate on (pros and cons of klp-convert in his experiments). > For example, from [3/10]: > > "For automatic resolution of livepatch relocations, a file called > symbols.klp is used. This file maps symbols within every compiled kernel > object allowing the identification of symbols whose name is unique, thus > relocation can be automatically inferred, or providing information that > helps developers when code annotation is required for solving the > matter." > > For the source based approach to livepatch preparation we're using > internally, this is not really needed: the entity generating the source > -- be it klp-ccp or the author doing it manually -- needs to examine the > target objects long before link resp. klp-convert time for which symbols > can be referenced from the livepatch and how (i.e. determine a potential > sympos). I would expect it works similar for kpatch-build conceptually, > albeit kpatch-build probably doesn't rely on any external utility like > klp-convert for the .klp.* relas generation at all. > Yes the conversion for kpatch-build the conversion is internal, though split in parts: The first step is during the binary comparison program (create-diff-object) to place all the potential klp-relocations into a temporary .kpatch_relocations section with info like dest, type, objname, etc. A second program, either create-klp-module [1] or the mostly deprecated create-kpatch-module, iterates through the intermediate .kpatch_relocation sections and converts to klp-relocation in the final module output. [1] https://github.com/dynup/kpatch/blob/master/kpatch-build/create-klp-module.c#L158 > So with that, I agree that merging the klp-convert patchset in its > current form with those potentially unused convenience features, > presumably born out of certain assumptions about a manual livepatch > preparation process, indeed can be argued about, probably. > > > However, OTOH, there's currently no means whatsoever to create those > .klp.* relas (*) (**) and I would like to propose resorting to a more > minimal utility doing only that single thing: to stubbornly create > .klp.* relas out of certain "regular" ones using a very simple > transformation rule and nothing else beyond that. The "stripped" > klp-convert would have no knowledge of the symbols available in the > livepatched target objects at all, i.e. there would be no symbols.klp > file or alike anymore. Instead, it would simply walk through all of a > livepatch object's SHN_UNDEF symbols of format > ".klp.sym.<loading-obj-name>.<foo-providing-mod>.some_foo,0" somewhen at > modpost time and > - rename the symbol to ".klp.sym.<foo-providing-mod>.some_foo,0" -- > shortening the name should always be feasible as far as strtab is > concerned. > - turn the symbol's SHN_UNDEF into SHN_LIVEPATCH > - move any relocation (initially created by the compiler with source > based lp preparation approaches) against this symbol into a separate, > newly created rela section with flag SHF_RELA_LIVEPATCH set and whose > name is of format > .klp.rela.<loading-obj-name>.<livepatch-obj-dst-section-name>. > Furthermore, the new .klp.rela section's ->sh_info needs to be made to > refer to the destination section. > If I understand the idea, it would be similar to the second kpatch-build step I mentioned above -- nothing fancy, just blindly convert symbols marked SHN_UNDEF and a given naming convention. FWIW, it may even be possible to retrofit kpatch-build to the format for testing -- that would offer the ability to test many real-world CVE fixups. > So, the only thing which would depend on the yet unspecified details of > the livepatch preparation process would be the creation of those > intermediate > ".klp.sym.<loading-obj-name>.<foo-providing-mod>.some_foo,0" SHN_UNDEF > symbols to be processed by klp-convert. For source based livepatch > preparation approaches, counting in the selftests, this can be easily > controlled by means of asm("...") alias specifications at the respective > declarations like in e.g. extern int foo > asm("\".klp.sym.<loading-obj-name>.<foo-providing-mod>.some_foo,0\""); > > > I imagine the first ones to benefit from having such a "stripped" > klp-convert available in the kernel tree would be new upstream selftests > for .klp.* rela coverage (like introduced with this here patchset > already) and for those some means of creating .klp.* relas would be > needed anyway. We (SUSE), and perhaps others as well, could integrate > this "stripped" klp-convert into our source based, production livepatch > preparation workflows right away, of course, and so we're obviously keen > on having it. Such a tool providing only the bare minimum would be > pretty much self-contained -- it would only need to hook into the > modpost Kbuild stage one way or the other -- and we could certainly > maintain it downstream out-of-tree, but that would potentially only > contribute to the current fragmentation around the livepatch creation > processes even more and there still wouldn't have a solution for the > upstream selftests. > I think you summed it up that having an in-tree user, a test suite at that, is a good motivation to maintain the conversion tool alongside. Logistically it may easier to prototype and spin bugfixes faster locally (esp. if it's supporting a quick CVE mitigation service), but hopefully the rapid-dev/fix phase is only temporary and the normal upstream-first development flow prevails. > What do you think, does it make sense to eventually have such a bare > minimum klp-convert merged in-tree, independently of the ongoing > discussion around the livepatch preparation processes, respectively (the > lack of) documentation around it? If yes, Lukas, now on CC, is > interested in this topic and would be willing to help out in any form > desired: either by contributing to Joe's work here or, if deemed more > feasible, to start out completely new from scratch -- dependent on your > opinion on the proposed, more minimal approach as well as on Joe's plans > around klp-convert. > I like the incremental approach of minimizing the complexity of the conversion tool. In fact, the hardest parts of this patchset (for me) revolved around the kbuild integration to generate that symbols file :) The next hardest where hypothetical problems with resolving to weird read-only sections and then approaching compiler optimizations.
On Wed, May 03, 2023 at 03:54:47PM -0400, Joe Lawrence wrote: > On 4/11/23 06:06, Nicolai Stange wrote: > > Josh Poimboeuf <jpoimboe@kernel.org> writes: > > > >> On Fri, Mar 17, 2023 at 04:29:48PM -0400, Joe Lawrence wrote: > >>> Have you tried retrofitting klp-convert into any real-world livepatch? > >>> I'm curious as to your observations on the overall experience, or > >>> thoughts on the sympos annotation style noted above. > >> > >> On a related note, the patch creation process (of which klp-convert > >> would be part of) needs to be documented. > >> > >> If I remember correctly, the proper safe usage of klp-convert requires a > >> kernel built with -flive-patching, plus some scripting and/or manual > >> processes. > > > > Not always, I think: -flive-patching or IPA optimizations in general > > aren't a concern in the context of data symbols. From a quick glance, it > > seems like the selftests introduced as part of this patchset are > > all restricted to this usecase. > > > > IIRC there is nothing currently stopping klp-convert from converting > function symbol relocations. That may be dangerous when taking > optimizations like sibling functions (and their sharing of stack) into > consideration. This is about the point I stopped to turn and see what > the real use cases may be. > > >> If nobody knows how to safely use it then there wouldn't be much value > >> in merging it. > > > > I tend to agree, but would put it a bit differently: the current > > implementation of klp-convert features quite some convenience logic, > > which, until the question of a documented livepatch preparation process > > has been settled, is not known yet to ever be of any use. > > > > Good observation and perhaps something that Marcos could elaborate on > (pros and cons of klp-convert in his experiments). > In my tests, I took the exact code generated by klp-ccp, and adapted it to not rely on kallsyms anymore, removing the symbol lookups. For data symbols, I changed it to be a extern variable instead of a pointer to it. For the function symbols, also removed the pointer lookup, and left only the function prototype, and it worked as expected. I was quite surprised that it worked quite well. But I agree with Nicolai that the tool itself could be shrunk into a smaller version. In our usage, klp-ccp knows all unexported functions and to which modules they belong, as it's currently used for the symbol lookup. I believe that kpatch-build also has similar information so all the symbols.klp and other Kbuild machinery could be avoided, making the tool responsible to only generate the klp relocations based on the undefined symbols, just as proposed by Nicolai.
Summary ------- Livepatches may use symbols which are not contained in its own scope, and, because of that, may end up compiled with relocations that will only be resolved during module load. Yet, when the referenced symbols are not exported, solving this relocation requires information on the object that holds the symbol (either vmlinux or modules) and its position inside the object, as an object may contain multiple symbols with the same name. Providing such information must be done accordingly to what is 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 in two different forms: (i) by relying on a symbol map, which is built during kernel compilation, to automatically infer the relocation targeted symbol, and, when such inference is not possible (ii) 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, add support for symbol mapping in the form of a symbols.klp file; add klp-convert tool; integrate klp-convert tool into kbuild; make livepatch modules discernible during kernel compilation pipeline; add data-structure and macros to enable users to annotate livepatch source code; make modpost stage compatible with livepatches; update livepatch-sample and update documentation. The patch was tested under three use-cases: use-case 1: There is a relocation in the lp that can be automatically resolved by klp-convert. For example. see the saved_command_line variable in lib/livepatch/test_klp_convert2.c. use-case 2: There is a relocation in the lp that cannot be automatically resolved, as the name of the respective symbol appears in multiple objects. The livepatch contains an annotation to enable a correct relocation. See the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in lib/livepatch/test_klp_convert{1,2}.c. use-case 3: There is a relocation in the lp that cannot be automatically resolved similarly as 2, but no annotation was provided in the livepatch, triggering an error during compilation. Reproducible by removing the KLP_MODULE_RELOC / KLP_SYMPOS annotation sections in lib/livepatch/test_klp_convert{1,2}.c. Selftests have been added to exercise these klp-convert use-cases through several tests. Testing ------- The patchset selftests build and execute on x86_64, s390x, and ppc64le for both default config (with added livepatch dependencies) and a larger RHEL-9-ish config. Using the Intel's Linux Kernel Performance tests's make.cross, klp-convert builds and processes livepatch .ko's for x86_64 ppc64le ppc32 s390 arm64 arches. Summary of changes in v7 ------------------------ - rebase for v6.2 - combine ("livepatch: Add klp-convert tool") with ("livepatch: Add klp-convert annotation helpers") - combine ("kbuild: Support for symbols.klp creation") with ("modpost: Integrate klp-convert") to simplify Kbuild magic [Petr, Nicolas] - klp-convert: add safe_snprintf() (-Wsign-compare) - klp-convert: fix -Wsign-compare warnings - klp-convert: use calloc() where appropriate - klp-convert: copy ELF e_flags - selftests: fix various build warnings - klp-convert: WARN msg simplification, failed sanity checks, and sympos comment [Marcos] - klp-convert: fix elf_write_file() error paths [Petr] Previous versions ----------------- RFC: https://lore.kernel.org/lkml/cover.1477578530.git.jpoimboe@redhat.com/ v2: https://lore.kernel.org/lkml/f52d29f7-7d1b-ad3d-050b-a9fa8878faf2@redhat.com/ v3: https://lore.kernel.org/lkml/20190410155058.9437-1-joe.lawrence@redhat.com/ v4: https://lore.kernel.org/lkml/20190509143859.9050-1-joe.lawrence@redhat.com/ v5: (not posted) https://github.com/joe-lawrence/klp-convert-tree/tree/klp-convert-v5-devel v6: https://lore.kernel.org/live-patching/20220216163940.228309-1-joe.lawrence@redhat.com/ Joe Lawrence (10): livepatch: Create and include UAPI headers livepatch: Add klp-convert tool kbuild/modpost: create symbols.klp and integrate klp-convert livepatch: Add sample livepatch module documentation: Update on livepatch elf format livepatch/selftests: add klp-convert livepatch/selftests: test multiple sections livepatch/selftests: add __asm__ symbol renaming examples livepatch/selftests: add data relocations test livepatch/selftests: add static keys test .gitignore | 2 + Documentation/dontdiff | 1 + Documentation/livepatch/livepatch.rst | 3 + Documentation/livepatch/module-elf-format.rst | 42 +- MAINTAINERS | 2 + Makefile | 16 +- include/linux/livepatch.h | 13 + include/uapi/linux/livepatch.h | 25 + kernel/livepatch/core.c | 4 +- lib/livepatch/Makefile | 12 + lib/livepatch/test_klp_convert.h | 45 + lib/livepatch/test_klp_convert1.c | 121 +++ lib/livepatch/test_klp_convert2.c | 110 +++ lib/livepatch/test_klp_convert_data.c | 190 ++++ lib/livepatch/test_klp_convert_keys.c | 91 ++ lib/livepatch/test_klp_convert_keys_mod.c | 52 + lib/livepatch/test_klp_convert_mod_a.c | 31 + lib/livepatch/test_klp_convert_mod_b.c | 19 + lib/livepatch/test_klp_convert_mod_c.c | 36 + lib/livepatch/test_klp_convert_sections.c | 120 +++ samples/livepatch/Makefile | 1 + .../livepatch/livepatch-annotated-sample.c | 93 ++ scripts/Makefile | 1 + scripts/Makefile.modfinal | 33 + scripts/Makefile.modpost | 5 + scripts/livepatch/.gitignore | 1 + scripts/livepatch/Makefile | 5 + scripts/livepatch/elf.c | 817 ++++++++++++++++ scripts/livepatch/elf.h | 74 ++ scripts/livepatch/klp-convert.c | 893 ++++++++++++++++++ scripts/livepatch/klp-convert.h | 47 + scripts/livepatch/list.h | 391 ++++++++ scripts/mod/modpost.c | 28 +- scripts/mod/modpost.h | 1 + .../selftests/livepatch/test-livepatch.sh | 403 ++++++++ 35 files changed, 3716 insertions(+), 12 deletions(-) create mode 100644 include/uapi/linux/livepatch.h create mode 100644 lib/livepatch/test_klp_convert.h create mode 100644 lib/livepatch/test_klp_convert1.c create mode 100644 lib/livepatch/test_klp_convert2.c create mode 100644 lib/livepatch/test_klp_convert_data.c create mode 100644 lib/livepatch/test_klp_convert_keys.c create mode 100644 lib/livepatch/test_klp_convert_keys_mod.c create mode 100644 lib/livepatch/test_klp_convert_mod_a.c create mode 100644 lib/livepatch/test_klp_convert_mod_b.c create mode 100644 lib/livepatch/test_klp_convert_mod_c.c create mode 100644 lib/livepatch/test_klp_convert_sections.c create mode 100644 samples/livepatch/livepatch-annotated-sample.c 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