| Message ID | 1458849640-22588-24-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > @@ -929,6 +932,33 @@ being loaded and requires an hypervisor build-id to match against. > The old code allows much more flexibility and an additional guard, > but is more complex to implement. > > +The second option which requires an build-id of the hypervisor > +is implemented in the Xen Project hypervisor. > + > +Specifically each payload has two build-id ELF notes: > + * The build-id of the payload itself (generated via --build-id). > + * The build-id of the payload it depends on (extracted from the > + the previous payload or hypervisor during build time). > + > +This means that the very first payload depends on the hypervisor > +build-id. So this is mean to be a singly linked chain, not something with branches and alike, allowing independent patches to be applied solely based on the base build ID? Is such a restriction not going to get in the way rather sooner than later? > +# Not Yet Done > + > +This is for further development of xSplice. > + > +## Goals > + > +The implementation must also have a mechanism for: > + > + * Be able to lookup in the Xen hypervisor the symbol names of functions from the ELF payload. > + * Be able to patch .rodata, .bss, and .data sections. > + * Further safety checks (blacklist of which functions cannot be patched, check > + the stack, make sure the payload is built with same compiler as hypervisor, > + and NMI/MCE handlers and do_nmi for right now - until an safe solution is found). > + * NOP out the code sequence if `new_size` is zero. > + * Deal with other relocation types: R_X86_64_[8,16,32,32S], R_X86_64_PC[8,16,64] in payload file. Does this belong here? Doesn't this duplicate something I saw earlier? > --- a/xen/common/version.c > +++ b/xen/common/version.c > @@ -70,10 +70,29 @@ const char *xen_deny(void) > /* Defined in linker script. */ > extern const Elf_Note __note_gnu_build_id_start[], > __note_gnu_build_id_end[]; > > +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len) > +{ > + /* Check if we really have a build-id. */ > + if ( NT_GNU_BUILD_ID != n->type ) > + return -ENODATA; > + > + /* Sanity check, name should be "GNU" for ld-generated build-id. */ > + if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 ) > + return -ENODATA; For the embedded notes this suffices as verification, but I question this being enough for a patch module: No part of the note should exceed the containing section. And maybe there are other things. > #else > > +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len) > +{ > + return -ENODATA; > +} What case is this needed for, considering that only xSplice code should be calling it, and that code depends on build ID availability. > +static int build_id_dep(struct payload *payload, bool_t ignore) > +{ > + const void *id = NULL; > + unsigned int len = 0; > + int rc; > + const char *name = "hypervisor"; > + > + ASSERT(payload->dep.len && payload->dep.p); > + > + /* First time user is against hypervisor. */ > + if ( ignore || list_empty(&applied_list) ) "ignore" is perhaps not the most descriptive name, as you aren't ignoring anything here. Maybe "internal"? And then maybe have the caller pass the argument using list_empty(&applied_list) instead of you checking it here? > --- a/xen/include/xen/version.h > +++ b/xen/include/xen/version.h > @@ -17,4 +17,7 @@ const char *xen_deny(void); > #include <xen/types.h> > int xen_build_id(const void **p, unsigned int *len); > > +#include <xen/elfstructs.h> > +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len); The #include is misplaced again, and I'm rather hesitant to see version.h gain this dependency. Couldn't this go into xen/elf.h? > --- a/xen/include/xen/xsplice.h > +++ b/xen/include/xen/xsplice.h > @@ -40,6 +40,11 @@ struct xsplice_symbol { > bool_t new_symbol; > }; > > +struct xsplice_build_id { > + const void *p; > + unsigned int len; > +}; This isn't being used outside of xsplice.c, so please define the structure there. Jan
On Mon, Apr 04, 2016 at 09:00:00AM -0600, Jan Beulich wrote: > >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: > > @@ -929,6 +932,33 @@ being loaded and requires an hypervisor build-id to match against. > > The old code allows much more flexibility and an additional guard, > > but is more complex to implement. > > > > +The second option which requires an build-id of the hypervisor > > +is implemented in the Xen Project hypervisor. > > + > > +Specifically each payload has two build-id ELF notes: > > + * The build-id of the payload itself (generated via --build-id). > > + * The build-id of the payload it depends on (extracted from the > > + the previous payload or hypervisor during build time). > > + > > +This means that the very first payload depends on the hypervisor > > +build-id. > > So this is mean to be a singly linked chain, not something with > branches and alike, allowing independent patches to be applied > solely based on the base build ID? Is such a restriction not going Correct. > to get in the way rather sooner than later? Here is what the design doc says: " ### xSplice interdependencies xSplice patches interdependencies are tricky. There are the ways this can be addressed: * A single large patch that subsumes and replaces all previous ones. Over the life-time of patching the hypervisor this large patch grows to accumulate all the code changes. * Hotpatch stack - where an mechanism exists that loads the hotpatches in the same order they were built in. We would need an build-id of the hypevisor to make sure the hot-patches are build against the correct build. * Payload containing the old code to check against that. That allows the hotpatches to be loaded indepedently (if they don't overlap) - or if the old code also containst previously patched code - even if they overlap. The disadvantage of the first large patch is that it can grow over time and not provide an bisection mechanism to identify faulty patches. The hot-patch stack puts stricts requirements on the order of the patches being loaded and requires an hypervisor build-id to match against. The old code allows much more flexibility and an additional guard, but is more complex to implement. The second option which requires an build-id of the hypervisor is implemented in the Xen Project hypervisor. " I was all for "old_code to check against" but that would incur quite a lot of implementation. The 'stacking' (suggested by Martin) is much easier to implement. I am hoping that in next major milestone the 'old code' checking can be implemented such that the admin has the choice to use both. > > > +# Not Yet Done > > + > > +This is for further development of xSplice. > > + > > +## Goals > > + > > +The implementation must also have a mechanism for: > > + > > + * Be able to lookup in the Xen hypervisor the symbol names of functions from the ELF payload. > > + * Be able to patch .rodata, .bss, and .data sections. > > + * Further safety checks (blacklist of which functions cannot be patched, check > > + the stack, make sure the payload is built with same compiler as hypervisor, > > + and NMI/MCE handlers and do_nmi for right now - until an safe solution is found). > > + * NOP out the code sequence if `new_size` is zero. > > + * Deal with other relocation types: R_X86_64_[8,16,32,32S], R_X86_64_PC[8,16,64] in payload file. > > Does this belong here? Doesn't this duplicate something I saw earlier? ? This is just shrinking the "TODO Goals" section and removing the: "- * An dependency mechanism for the payloads. To use that information to load: - - The appropiate payload. To verify that payload is built against the - hypervisor. This can be done via the `build-id` - or via providing an copy of the old code - so that the hypervisor can - verify it against the code in memory. - - To construct an appropiate order of payloads to load in case they - depend on each other. " The other TODOs are not added.
>>> On 04.04.16 at 22:01, <konrad.wilk@oracle.com> wrote: > On Mon, Apr 04, 2016 at 09:00:00AM -0600, Jan Beulich wrote: >> >>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >> > @@ -929,6 +932,33 @@ being loaded and requires an hypervisor build-id to match against. >> > The old code allows much more flexibility and an additional guard, >> > but is more complex to implement. >> > >> > +The second option which requires an build-id of the hypervisor >> > +is implemented in the Xen Project hypervisor. >> > + >> > +Specifically each payload has two build-id ELF notes: >> > + * The build-id of the payload itself (generated via --build-id). >> > + * The build-id of the payload it depends on (extracted from the >> > + the previous payload or hypervisor during build time). >> > + >> > +This means that the very first payload depends on the hypervisor >> > +build-id. >> >> So this is mean to be a singly linked chain, not something with >> branches and alike, allowing independent patches to be applied >> solely based on the base build ID? Is such a restriction not going > > Correct. >> to get in the way rather sooner than later? > > Here is what the design doc says: > > " > ### xSplice interdependencies > > xSplice patches interdependencies are tricky. > > There are the ways this can be addressed: > * A single large patch that subsumes and replaces all previous ones. > Over the life-time of patching the hypervisor this large patch > grows to accumulate all the code changes. > * Hotpatch stack - where an mechanism exists that loads the hotpatches > in the same order they were built in. We would need an build-id > of the hypevisor to make sure the hot-patches are build against the > correct build. > * Payload containing the old code to check against that. That allows > the hotpatches to be loaded indepedently (if they don't overlap) - or > if the old code also containst previously patched code - even if they > overlap. > > The disadvantage of the first large patch is that it can grow over > time and not provide an bisection mechanism to identify faulty patches. > > The hot-patch stack puts stricts requirements on the order of the patches > being loaded and requires an hypervisor build-id to match against. > > The old code allows much more flexibility and an additional guard, > but is more complex to implement. > > The second option which requires an build-id of the hypervisor > is implemented in the Xen Project hypervisor. > > " > > I was all for "old_code to check against" but that would incur quite a lot > of implementation. The 'stacking' (suggested by Martin) is much easier > to implement. I am hoping that in next major milestone the 'old code' > checking > can be implemented such that the admin has the choice to use both. In which case could both the design doc and the commit message be made say that this is intended to be a temporary thing, to be replaced by the more flexible model? >> > +# Not Yet Done >> > + >> > +This is for further development of xSplice. >> > + >> > +## Goals >> > + >> > +The implementation must also have a mechanism for: >> > + >> > + * Be able to lookup in the Xen hypervisor the symbol names of functions from the ELF payload. >> > + * Be able to patch .rodata, .bss, and .data sections. >> > + * Further safety checks (blacklist of which functions cannot be patched, check >> > + the stack, make sure the payload is built with same compiler as hypervisor, >> > + and NMI/MCE handlers and do_nmi for right now - until an safe solution is found). >> > + * NOP out the code sequence if `new_size` is zero. >> > + * Deal with other relocation types: R_X86_64_[8,16,32,32S], R_X86_64_PC[8,16,64] in payload file. >> >> Does this belong here? Doesn't this duplicate something I saw earlier? > > ? This is just shrinking the "TODO Goals" section and removing the: > "- * An dependency mechanism for the payloads. To use that information to load: > - - The appropiate payload. To verify that payload is built against the > - hypervisor. This can be done via the `build-id` > - or via providing an copy of the old code - so that the hypervisor can > - verify it against the code in memory. > - - To construct an appropiate order of payloads to load in case they > - depend on each other. > " > > The other TODOs are not added. Oh, I didn't realize that the block gets moved. That's certainly not very fortunate for review (the base document structure would better remain the same). Jan
> > --- a/xen/include/xen/version.h > > +++ b/xen/include/xen/version.h > > @@ -17,4 +17,7 @@ const char *xen_deny(void); > > #include <xen/types.h> > > int xen_build_id(const void **p, unsigned int *len); > > > > +#include <xen/elfstructs.h> > > +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len); > > The #include is misplaced again, and I'm rather hesitant to see > version.h gain this dependency. Couldn't this go into xen/elf.h? Sadly no. If I remove these two includes (xen/types.h and xen/elfstructs.h) the compile breaks. It breaks for kernel.c, console,c, setup.c and so on - complaining about unknown type name 'Elf_Note'. I was thinking to do: const Elf_Note; But that didn't work out well. I think it is tied in to the #defines.. I could make it a void pointer too and just cast it inside xen_build_id_check to 'const Elf_Note'.
>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 04/06/16 10:05 PM >>> >> > --- a/xen/include/xen/version.h >> > +++ b/xen/include/xen/version.h >> > @@ -17,4 +17,7 @@ const char *xen_deny(void); >> > #include <xen/types.h> >> > int xen_build_id(const void **p, unsigned int *len); >> > >> > +#include <xen/elfstructs.h> >> > +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len); >> >> The #include is misplaced again, and I'm rather hesitant to see >> version.h gain this dependency. Couldn't this go into xen/elf.h? > >Sadly no. If I remove these two includes (xen/types.h and >xen/elfstructs.h) the compile breaks. It breaks for kernel.c, console,c, >setup.c and so on - complaining about unknown type name 'Elf_Note'. I guess there's a misunderstanding of my use of "this" here: I meant the function declaration to get moved over. I can't see how that would cause compilation problems. Jan
On 04/04/2016 09:01 PM, Konrad Rzeszutek Wilk wrote: > On Mon, Apr 04, 2016 at 09:00:00AM -0600, Jan Beulich wrote: >>>>> On 24.03.16 at 21:00, <konrad.wilk@oracle.com> wrote: >>> @@ -929,6 +932,33 @@ being loaded and requires an hypervisor build-id to match against. >>> The old code allows much more flexibility and an additional guard, >>> but is more complex to implement. >>> >>> +The second option which requires an build-id of the hypervisor >>> +is implemented in the Xen Project hypervisor. >>> + >>> +Specifically each payload has two build-id ELF notes: >>> + * The build-id of the payload itself (generated via --build-id). >>> + * The build-id of the payload it depends on (extracted from the >>> + the previous payload or hypervisor during build time). >>> + >>> +This means that the very first payload depends on the hypervisor >>> +build-id. >> >> So this is mean to be a singly linked chain, not something with >> branches and alike, allowing independent patches to be applied >> solely based on the base build ID? Is such a restriction not going > > Correct. >> to get in the way rather sooner than later? > > Here is what the design doc says: > > " > ### xSplice interdependencies > > xSplice patches interdependencies are tricky. > > There are the ways this can be addressed: > * A single large patch that subsumes and replaces all previous ones. > Over the life-time of patching the hypervisor this large patch > grows to accumulate all the code changes. > * Hotpatch stack - where an mechanism exists that loads the hotpatches > in the same order they were built in. We would need an build-id > of the hypevisor to make sure the hot-patches are build against the > correct build. > * Payload containing the old code to check against that. That allows > the hotpatches to be loaded indepedently (if they don't overlap) - or > if the old code also containst previously patched code - even if they > overlap. > > The disadvantage of the first large patch is that it can grow over > time and not provide an bisection mechanism to identify faulty patches. > > The hot-patch stack puts stricts requirements on the order of the patches > being loaded and requires an hypervisor build-id to match against. > > The old code allows much more flexibility and an additional guard, > but is more complex to implement. > > The second option which requires an build-id of the hypervisor > is implemented in the Xen Project hypervisor. > > " > > I was all for "old_code to check against" but that would incur quite a lot > of implementation. The 'stacking' (suggested by Martin) is much easier > to implement. I am hoping that in next major milestone the 'old code' checking > can be implemented such that the admin has the choice to use both. I don't think checking old code provides any real safety though. What if the function I'm replacing is unchanged (so it passes the old code check) but it calls some other function whose behavior has changed? It's somewhat telling that the publicly available binary patches for kSplice always use a linear stack of patches with dependency management done in userspace, despite having old code checking. What kSplice use in practice is exactly what is implemented here; a linear stack of patches using some sort of identifier (build-id/uuid).
>>> On 08.04.16 at 18:15, <ross.lagerwall@citrix.com> wrote: > On 04/04/2016 09:01 PM, Konrad Rzeszutek Wilk wrote: >> I was all for "old_code to check against" but that would incur quite a lot >> of implementation. The 'stacking' (suggested by Martin) is much easier >> to implement. I am hoping that in next major milestone the 'old code' checking >> can be implemented such that the admin has the choice to use both. > > I don't think checking old code provides any real safety though. What if > the function I'm replacing is unchanged (so it passes the old code > check) but it calls some other function whose behavior has changed? That's identical to an entirely bad patch imo. Jan
diff --git a/.gitignore b/.gitignore index 8dc76b5..f1cadcf 100644 --- a/.gitignore +++ b/.gitignore @@ -247,6 +247,7 @@ xen/arch/x86/efi/disabled xen/arch/x86/efi/mkreloc xen/arch/x86/test/config.h xen/arch/x86/test/xen_hello_world.xsplice +xen/arch/x86/test/xen_bye_world.xsplice xen/arch/*/efi/boot.c xen/arch/*/efi/compat.c xen/arch/*/efi/efi.h diff --git a/Config.mk b/Config.mk index c8e89fe..80629e3 100644 --- a/Config.mk +++ b/Config.mk @@ -134,6 +134,7 @@ ifeq ($(call ld-ver-build-id,$(LD)),n) build_id_linker := else CFLAGS += -DBUILD_ID +export XEN_HAS_BUILD_ID=y build_id_linker := --build-id=sha1 endif diff --git a/docs/misc/xsplice.markdown b/docs/misc/xsplice.markdown index be9cd71..8b9ce9c 100644 --- a/docs/misc/xsplice.markdown +++ b/docs/misc/xsplice.markdown @@ -283,8 +283,7 @@ The xSplice core code loads the payload as a standard ELF binary, relocates it and handles the architecture-specifc sections as needed. This process is much like what the Linux kernel module loader does. -The payload contains a section (xsplice_patch_func) with an array of structures -describing the functions to be patched: +The payload contains at least three sections: It optionally may contain the address of functions to be called right before being applied and after being reverted: @@ -292,6 +291,15 @@ being applied and after being reverted: * `.xsplice.hooks.unload` - an array of function pointers. + * `.xsplice.funcs` - which is an array of xsplice_patch_func structures. + * `.xsplice.depends` - which is an ELF Note that describes what the payload + depends on. + * `.note.gnu.build-id` - the build-id of this payload. + +### .xsplice.funcs + +The `.xsplice.funcs` contains an array of xsplice_patch_func structures +which describe the functions to be patched: <pre> struct xsplice_patch_func { const char *name; @@ -333,7 +341,7 @@ When reverting a patch, the hypervisor iterates over each `xsplice_patch_func` and the core code copies the data from the undo buffer (private internal copy) to `old_addr`. -### Example +### Example of .xsplice.funcs A simple example of what a payload file can be: @@ -379,10 +387,29 @@ Each entry in this array is eight bytes. The type definition of the function are as follow: + <pre> typedef void (*xsplice_loadcall_t)(void); typedef void (*xsplice_unloadcall_t)(void); </pre> + +### .xsplice.depends and .note.gnu.build-id + +To support dependencies checking and safe loading (to load the +appropiate payload against the right hypervisor) there is a need +to embbed an build-id dependency. + +This is done by the payload containing an section `.xsplice.depends` +which follows the format of an ELF Note. The contents of this +(name, and description) are specific to the linker utilized to +build the hypevisor and payload. + +If GNU linker is used then the name is `GNU` and the description +is an NT_GNU_BUILD_ID type ID. The description can be an SHA1 +checksum, MD5 checksum or any unique value. + +The size of these structures varies with the --build-id linker option. + ## Hypercalls We will employ the sub operations of the system management hypercall (sysctl). @@ -879,30 +906,6 @@ This is implemented in the Xen Project hypervisor. Only the privileged domain should be allowed to do this operation. - -# Not Yet Done - -This is for further development of xSplice. - -## Goals - -The implementation must also have a mechanism for: - - * An dependency mechanism for the payloads. To use that information to load: - - The appropiate payload. To verify that payload is built against the - hypervisor. This can be done via the `build-id` - or via providing an copy of the old code - so that the hypervisor can - verify it against the code in memory. - - To construct an appropiate order of payloads to load in case they - depend on each other. - * Be able to lookup in the Xen hypervisor the symbol names of functions from the ELF payload. - * Be able to patch .rodata, .bss, and .data sections. - * Further safety checks (blacklist of which functions cannot be patched, check - the stack, make sure the payload is built with same compiler as hypervisor, - and NMI/MCE handlers and do_nmi for right now - until an safe solution is found). - * NOP out the code sequence if `new_size` is zero. - * Deal with other relocation types: R_X86_64_[8,16,32,32S], R_X86_64_PC[8,16,64] in payload file. - ### xSplice interdependencies xSplice patches interdependencies are tricky. @@ -929,6 +932,33 @@ being loaded and requires an hypervisor build-id to match against. The old code allows much more flexibility and an additional guard, but is more complex to implement. +The second option which requires an build-id of the hypervisor +is implemented in the Xen Project hypervisor. + +Specifically each payload has two build-id ELF notes: + * The build-id of the payload itself (generated via --build-id). + * The build-id of the payload it depends on (extracted from the + the previous payload or hypervisor during build time). + +This means that the very first payload depends on the hypervisor +build-id. + +# Not Yet Done + +This is for further development of xSplice. + +## Goals + +The implementation must also have a mechanism for: + + * Be able to lookup in the Xen hypervisor the symbol names of functions from the ELF payload. + * Be able to patch .rodata, .bss, and .data sections. + * Further safety checks (blacklist of which functions cannot be patched, check + the stack, make sure the payload is built with same compiler as hypervisor, + and NMI/MCE handlers and do_nmi for right now - until an safe solution is found). + * NOP out the code sequence if `new_size` is zero. + * Deal with other relocation types: R_X86_64_[8,16,32,32S], R_X86_64_PC[8,16,64] in payload file. + ### Handle inlined __LINE__ This problem is related to hotpatch construction diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile index 22fcb14..8d355fb 100644 --- a/xen/arch/x86/test/Makefile +++ b/xen/arch/x86/test/Makefile @@ -7,13 +7,16 @@ CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}') ifdef CONFIG_XSPLICE XSPLICE := xen_hello_world.xsplice +XSPLICE_BYE := xen_bye_world.xsplice default: xsplice install: xsplice $(INSTALL_DATA) $(XSPLICE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE) + $(INSTALL_DATA) $(XSPLICE_BYE) $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_BYE) uninstall: rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE) + rm -f $(DESTDIR)$(DEBUG_DIR)/$(XSPLICE_BYE) else @@ -25,7 +28,7 @@ endif .PHONY: clean clean:: - rm -f *.o .*.o.d $(XSPLICE) config.h + rm -f *.o .*.o.d $(XSPLICE) config.h build_id.o # # To compute these values we need the binary files: xen-syms @@ -37,15 +40,43 @@ clean:: .PHONY: config.h config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version) config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world) -config.h: xen_hello_world_func.o +config.h: xen_hello_world_func.o xen_bye_world_func.o (set -e; \ echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \ echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@ +# +# This target is only accessible if CONFIG_XSPLICE is defined, which +# depends on $(build_id_linker) being available. Hence we do not +# need any checks. +# +.PHONY: build_id.o +build_id.o: + $(OBJCOPY) -O binary --only-section=.note $(BASEDIR)/xen-syms $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ + --rename-section=.data=.xsplice.depends -S $@.bin $@ + rm -f $@.bin + +# +# Extract the build-id of the xen_hello_world.xsplice +# (which xen_bye_world will depend on). +# +.PHONY: hello_world_build_id.o +hello_world_build_id.o: + $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(XSPLICE) $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \ + --rename-section=.data=.xsplice.depends -S $@.bin $@ + rm -f $@.bin + .PHONY: xsplice -xsplice: config.h +xsplice: config.h build_id.o # Need to have these done in sequential order $(MAKE) -f $(BASEDIR)/Rules.mk xen_hello_world_func.o $(MAKE) -f $(BASEDIR)/Rules.mk xen_hello_world.o - $(LD) $(LDFLAGS) -r -o $(XSPLICE) xen_hello_world_func.o \ - xen_hello_world.o + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE) \ + xen_hello_world_func.o xen_hello_world.o build_id.o + $(MAKE) -f $(BASEDIR)/Rules.mk xen_bye_world_func.o + $(MAKE) -f $(BASEDIR)/Rules.mk xen_bye_world.o + $(MAKE) -f $(BASEDIR)/Rules.mk hello_world_build_id.o + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE_BYE) \ + xen_bye_world_func.o xen_bye_world.o hello_world_build_id.o diff --git a/xen/arch/x86/test/xen_bye_world.c b/xen/arch/x86/test/xen_bye_world.c new file mode 100644 index 0000000..f9e3b8a --- /dev/null +++ b/xen/arch/x86/test/xen_bye_world.c @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/types.h> +#include <xen/xsplice_patch.h> +#include <xen/xsplice.h> +#include "config.h" +#include <xen/lib.h> + +static char xen_bye_world_name[] = "xen_bye_world"; +extern const char *xen_bye_world(void); + +/* External symbol. */ +extern const char *xen_extra_version(void); + +struct xsplice_patch_func __section(".xsplice.funcs") xsplice_xen_bye_world = { + .name = xen_bye_world_name, + .new_addr = (unsigned long)(xen_bye_world), + .old_addr = (unsigned long)(xen_extra_version), + .new_size = NEW_CODE_SZ, + .old_size = OLD_CODE_SZ, +}; + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/arch/x86/test/xen_bye_world_func.c b/xen/arch/x86/test/xen_bye_world_func.c new file mode 100644 index 0000000..574268c --- /dev/null +++ b/xen/arch/x86/test/xen_bye_world_func.c @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/config.h> +#include <xen/types.h> +#include <asm/nops.h> +#include <asm/alternative.h> + +/* Our replacement function for xen_hello_world. */ +const char *xen_bye_world(void) +{ + return "Bye World!"; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/Kconfig b/xen/common/Kconfig index d80dddb..0b8917c 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -54,6 +54,10 @@ config HAS_GDBSX config HAS_IOPORTS bool +config HAS_BUILD_ID + string + option env="XEN_HAS_BUILD_ID" + # Enable/Disable kexec support config KEXEC bool "kexec support" @@ -186,7 +190,7 @@ endmenu config XSPLICE bool "xSplice live patching support" default n - depends on X86 + depends on X86 && HAS_BUILD_ID = "y" ---help--- Allows a running Xen hypervisor to be dynamically patched using binary patches without rebooting. This is primarily used to binarily diff --git a/xen/common/version.c b/xen/common/version.c index 35078c7..b722a38 100644 --- a/xen/common/version.c +++ b/xen/common/version.c @@ -70,10 +70,29 @@ const char *xen_deny(void) /* Defined in linker script. */ extern const Elf_Note __note_gnu_build_id_start[], __note_gnu_build_id_end[]; +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len) +{ + /* Check if we really have a build-id. */ + if ( NT_GNU_BUILD_ID != n->type ) + return -ENODATA; + + /* Sanity check, name should be "GNU" for ld-generated build-id. */ + if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 ) + return -ENODATA; + + if ( len ) + *len = n->descsz; + if ( p ) + *p = ELFNOTE_DESC(n); + + return 0; +} + int xen_build_id(const void **p, unsigned int *len) { const Elf_Note *n = __note_gnu_build_id_start; static bool_t checked = 0; + int rc; if ( checked ) { @@ -89,23 +108,20 @@ int xen_build_id(const void **p, unsigned int *len) if ( &n[1] > __note_gnu_build_id_end ) return -ENODATA; - /* Check if we really have a build-id. */ - if ( NT_GNU_BUILD_ID != n->type ) - return -ENODATA; - - /* Sanity check, name should be "GNU" for ld-generated build-id. */ - if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 ) - return -ENODATA; - - *len = n->descsz; - *p = ELFNOTE_DESC(n); + rc = xen_build_id_check(n, p, len); + if ( !rc ) + checked = 1; - checked = 1; - return 0; + return rc; } #else +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len) +{ + return -ENODATA; +} + int xen_build_id(const void **p, unsigned int *len) { return -ENODATA; diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c index ec47f31..d7a65fe 100644 --- a/xen/common/xsplice.c +++ b/xen/common/xsplice.c @@ -4,6 +4,7 @@ */ #include <xen/cpu.h> +#include <xen/elf.h> #include <xen/err.h> #include <xen/guest_access.h> #include <xen/keyhandler.h> @@ -68,6 +69,8 @@ struct payload { xsplice_unloadcall_t *unload_funcs; /* load and unload of the payload. */ unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */ unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */ + struct xsplice_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */ + struct xsplice_build_id dep; /* ELFNOTE_DESC(.xsplice.depends). */ char name[XEN_XSPLICE_NAME_SIZE]; /* Name of it. */ }; @@ -440,7 +443,9 @@ static int check_special_sections(struct payload *payload, struct xsplice_elf *elf) { unsigned int i; - static const char *const names[] = { ".xsplice.funcs" }; + static const char *const names[] = { ".xsplice.funcs" , + ".xsplice.depends", + ".note.gnu.build-id"}; for ( i = 0; i < ARRAY_SIZE(names); i++ ) { @@ -461,6 +466,8 @@ static int check_special_sections(struct payload *payload, return 0; } +#define NT_GNU_BUILD_ID 3 + static int prepare_payload(struct payload *payload, struct xsplice_elf *elf) { @@ -468,6 +475,7 @@ static int prepare_payload(struct payload *payload, unsigned int i; struct xsplice_patch_func *f; struct virtual_region *region; + Elf_Note *n; sec = xsplice_elf_sec_by_name(elf, ".xsplice.funcs"); if ( sec ) @@ -545,6 +553,33 @@ static int prepare_payload(struct payload *payload, payload->n_unload_funcs = sec->sec->sh_size / (sizeof *payload->unload_funcs); } + sec = xsplice_elf_sec_by_name(elf, ".note.gnu.build-id"); + if ( sec ) + { + n = (Elf_Note *)sec->load_addr; + if ( sec->sec->sh_size <= sizeof *n ) + return -EINVAL; + + if ( xen_build_id_check(n, &payload->id.p, &payload->id.len) ) + return -EINVAL; + + if ( !payload->id.len || !payload->id.p ) + return -EINVAL; + } + + sec = xsplice_elf_sec_by_name(elf, ".xsplice.depends"); + { + n = (Elf_Note *)sec->load_addr; + if ( sec->sec->sh_size <= sizeof *n ) + return -EINVAL; + + if ( xen_build_id_check(n, &payload->dep.p, &payload->dep.len) ) + return -EINVAL; + + if ( !payload->dep.len || !payload->dep.p ) + return -EINVAL; + } + /* Setup the virtual region with proper data. */ region = &payload->region; @@ -1261,6 +1296,54 @@ void check_for_xsplice_work(void) } } +/* + * Only allow dependent payload is applied on top of the correct + * build-id. + * + * This enforces an stacking order - the first payload MUST be against the + * hypervisor. The second against the first payload, and so on. + * + * Unless the 'ignore' parameter is used - in which case we only + * check against the hypervisor. + */ +static int build_id_dep(struct payload *payload, bool_t ignore) +{ + const void *id = NULL; + unsigned int len = 0; + int rc; + const char *name = "hypervisor"; + + ASSERT(payload->dep.len && payload->dep.p); + + /* First time user is against hypervisor. */ + if ( ignore || list_empty(&applied_list) ) + { + rc = xen_build_id(&id, &len); + if ( rc ) + return rc; + } + else + { + /* We should be against the last applied one. */ + struct payload *data = list_last_entry(&applied_list, struct payload, + applied_list); + + id = data->id.p; + len = data->id.len; + name = data->name; + } + + if ( payload->dep.len != len || + memcmp(id, payload->dep.p, len) ) + { + dprintk(XENLOG_DEBUG, "%s%s: check against %s build-id failed!\n", + XSPLICE, payload->name, name); + return -EINVAL; + } + + return 0; +} + static int xsplice_action(xen_sysctl_xsplice_action_t *action) { struct payload *data; @@ -1304,6 +1387,18 @@ static int xsplice_action(xen_sysctl_xsplice_action_t *action) case XSPLICE_ACTION_REVERT: if ( data->state == XSPLICE_STATE_APPLIED ) { + struct payload *p = list_last_entry(&applied_list, struct payload, + applied_list); + + ASSERT(p); + /* We should be the last applied one. */ + if ( p != data ) + { + dprintk(XENLOG_DEBUG, "%s%s: can't unload. Top is %s!\n", + XSPLICE, data->name, p->name); + rc = -EBUSY; + break; + } data->rc = -EAGAIN; rc = schedule_work(data, action->cmd, action->timeout); } @@ -1312,6 +1407,9 @@ static int xsplice_action(xen_sysctl_xsplice_action_t *action) case XSPLICE_ACTION_APPLY: if ( data->state == XSPLICE_STATE_CHECKED ) { + rc = build_id_dep(data, 0 /* against top one. */); + if ( rc ) + break; data->rc = -EAGAIN; rc = schedule_work(data, action->cmd, action->timeout); } @@ -1320,6 +1418,9 @@ static int xsplice_action(xen_sysctl_xsplice_action_t *action) case XSPLICE_ACTION_REPLACE: if ( data->state == XSPLICE_STATE_CHECKED ) { + rc = build_id_dep(data, 1 /* against hypervisor. */); + if ( rc ) + break; data->rc = -EAGAIN; rc = schedule_work(data, action->cmd, action->timeout); } diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h index 4b57e18..a06bb61 100644 --- a/xen/include/xen/version.h +++ b/xen/include/xen/version.h @@ -17,4 +17,7 @@ const char *xen_deny(void); #include <xen/types.h> int xen_build_id(const void **p, unsigned int *len); +#include <xen/elfstructs.h> +int xen_build_id_check(const Elf_Note *n, const void **p, unsigned int *len); + #endif /* __XEN_VERSION_H__ */ diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h index 47ef1c2..fa50fb5 100644 --- a/xen/include/xen/xsplice.h +++ b/xen/include/xen/xsplice.h @@ -40,6 +40,11 @@ struct xsplice_symbol { bool_t new_symbol; }; +struct xsplice_build_id { + const void *p; + unsigned int len; +}; + int xsplice_op(struct xen_sysctl_xsplice_op *); void check_for_xsplice_work(void); bool_t is_patch(const void *addr);
We now expect that the ELF payloads be built with the --build-id. Also the .xsplice.deps section has to have the contents of the hypervisor (or a preceding payload) build-id. We already have the code to verify the Elf_Note build-id so export parts of it. This dependency means the hypervisor MUST be compiled with --build-id - so we gate the build of xSplice on the availability of said functionality. This does not impact the ordering of how the payloads can be loaded, but it does enforce an STRICT ordering when the payloads are applied. Also the REPLACE is special - we need to check that its dependency against the hypervisor - not the last applied patch. To make this easier to test we also add an extra test-case to be used - which can only be applied on top of the xen_hello_world payload. As in, one can apply xen_hello_world and then xen_bye_world on top of that. Not the other way. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Keir Fraser <keir@xen.org> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> v3: First time included. v4: Andrew fix against the build_id.o mutilations. Andrew fix to not include extra symbols in binary.id v5: s/ssize_t/unsigned int/ --- .gitignore | 1 + Config.mk | 1 + docs/misc/xsplice.markdown | 84 ++++++++++++++++++--------- xen/arch/x86/test/Makefile | 41 +++++++++++-- xen/arch/x86/test/xen_bye_world.c | 34 +++++++++++ xen/arch/x86/test/xen_bye_world_func.c | 25 ++++++++ xen/common/Kconfig | 6 +- xen/common/version.c | 40 +++++++++---- xen/common/xsplice.c | 103 ++++++++++++++++++++++++++++++++- xen/include/xen/version.h | 3 + xen/include/xen/xsplice.h | 5 ++ 11 files changed, 297 insertions(+), 46 deletions(-) create mode 100644 xen/arch/x86/test/xen_bye_world.c create mode 100644 xen/arch/x86/test/xen_bye_world_func.c