Re: [Xen-devel] [PATCH v10 21/24] xsplice: Stacking build-id dependency checking.

[Konrad Rzeszutek Wilk]

On Mon, May 02, 2016 at 12:37:09AM -0600, Jan Beulich wrote:
> >>> On 27.04.16 at 21:27,  wrote:
> > @@ -506,6 +518,37 @@ static int prepare_payload(struct payload *payload,
> >  }
> >  }
> >  
> > +sec = xsplice_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
> > +if ( sec )
> > +{
> > +n = sec->load_addr;
> > +
> > +if ( sec->sec->sh_size <= sizeof(*n) )
> > +return -EINVAL;
> > +
> > +if ( xen_build_id_check(n, sec->sec->sh_size,
> > +>id.p, >id.len) )
> > +return -EINVAL;
> > +
> > +if ( !payload->id.len || !payload->id.p )
> > +return -EINVAL;
> > +}
> > +
> > +sec = xsplice_elf_sec_by_name(elf, ELF_XSPLICE_DEPENDS);
> > +{
> 
> Looks like an "if ( sec )" got lost here.



Patch sent. Thank you for spotting that!
> 
> Jan
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 21/24] xsplice: Stacking build-id dependency checking.

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> @@ -506,6 +518,37 @@ static int prepare_payload(struct payload *payload,
>  }
>  }
>  
> +sec = xsplice_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
> +if ( sec )
> +{
> +n = sec->load_addr;
> +
> +if ( sec->sec->sh_size <= sizeof(*n) )
> +return -EINVAL;
> +
> +if ( xen_build_id_check(n, sec->sec->sh_size,
> +>id.p, >id.len) )
> +return -EINVAL;
> +
> +if ( !payload->id.len || !payload->id.p )
> +return -EINVAL;
> +}
> +
> +sec = xsplice_elf_sec_by_name(elf, ELF_XSPLICE_DEPENDS);
> +{

Looks like an "if ( sec )" got lost here.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v10 21/24] xsplice: Stacking build-id dependency checking.

[Jan Beulich]

>>> On 27.04.16 at 21:27,  wrote:
> --- a/xen/arch/x86/test/Makefile
> +++ b/xen/arch/x86/test/Makefile
> @@ -6,17 +6,20 @@ CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ 
> print "0x"$$2}')
>  .PHONY: default
>  
>  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)
>  
>  .PHONY: clean
>  clean::
> - rm -f *.o .*.o.d $(XSPLICE) config.h
> + rm -f *.o .*.o.d $(XSPLICE) $(XSPLICE_BYE) config.h *.bin

While you can't do this in the uninstall target above, here using
*.xsplice would seem to be better (and then maybe do this right
away in the patch introducing the rule).

> +$(XSPLICE_BYE): $(XSPLICE) config.h xen_bye_world_func.o xen_bye_world.o 
> hello_world_note.o
> + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(XSPLICE_BYE) \
> + $(filter %.o,$^)

Same here regarding config.h and $(filter ...), i.e. with that adjusted
Acked-by: Jan Beulich 

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH v10 21/24] xsplice: Stacking build-id dependency checking.

[Konrad Rzeszutek Wilk]

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.

We also print the dependency and payloads build_in the keyhandler.

Signed-off-by: Konrad Rzeszutek Wilk 

---
Cc: Keir Fraser 
Cc: Jan Beulich 
Cc: Andrew Cooper 

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/
v6: s/an NT_GNU../a NT_GNU/
   - Squash "xsplice: Print dependency and payloads build_id in the keyhandler"
 in this patch.
   - Add in xen_build_id_check size of section for better checking.
v7: Added Andrew's reviewed-by.
Change the .name in test-case to adhere to spec.
Dropped NT_GNU_BUILD_ID and moved that to earlier patch
(build_id: Provide ld-embedded build-ids)
Amended spec and code to only have one of .xsplice.depends and
.note.gnu.build-id
Expanded comment about note.o and why we don't use arch/x86/note.o.bin
Moved xen_build_id_check definition to xsplice.h from version.h
(and dropping the #include's in version.h)
Sort header files in tests.
v8:
 - Change two of the dprinkt from XENLOG_DEBUG to XENLOG_ERR
v9:
 - Dropped the (unsigned long) casts since we use void.
 - Make the .xsplice_depends and .note.gnu_build_id be #defines.
 - Make the build section use $(XSPLICE_BYE)
 - Make the testcase include 
 - Made comparisons on descsz and namesz a bit different (overflow
   checks, against value of 4, and against size)
v10 - inline patches to response to v9:
 - Use filter() to only link .o files.
 - Make dependency for xen_bye_world.o be config.h only
 - Make overflow check for n->namesz and n->descsz be proper
 - Check n->namesz against less than 4.
 - Change check against header of Elf_Note
 - Calculate size (in bytes) of Elf_Note using pointer arithmetic.
 - Dropped Andrew' Reviewed-by
---
 .gitignore |   1 +
 Config.mk  |   1 +
 docs/misc/xsplice.markdown |  99 ++--
 xen/arch/x86/test/Makefile |  46 +++--
 xen/arch/x86/test/xen_bye_world.c  |  34 ++
 xen/arch/x86/test/xen_bye_world_func.c |  22 +++
 xen/common/Kconfig |   6 +-
 xen/common/version.c   |  45 +
 xen/common/xsplice.c   | 117 -
 xen/include/xen/xsplice.h  |   4 ++
 10 files changed, 323 insertions(+), 52 deletions(-)
 create mode 100644 xen/arch/x86/test/xen_bye_world.c
 create mode 100644 xen/arch/x86/test/xen_bye_world_func.c

diff --git a/.gitignore b/.gitignore
index 4a81f43..88cec1d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -248,6 +248,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 759f3c7..23a1b74 100644
--- a/Config.mk
+++ b/Config.mk
@@ -137,6 +137,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 35ebc28..4a98be1 100644
--- a/docs/misc/xsplice.markdown
+++ b/docs/misc/xsplice.markdown
@@ -283,8 +283,17 @@ 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:
+
+ * `.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. **MUST** have one.
+ *  `.note.gnu.build-id` - the build-id of this payload. **MUST** have one.
+
+###