[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.



Hi Konrad,

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
By default when using objcopy we lose the alignment when we copy it from 
xen-syms -
with the result that alignment (on ARM32 for example) can be 1:

   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf 
Al
..
   [ 6] .livepatch.depend PROGBITS        00000000 000093 000024 00   A  0   0  
1

That, combined with wacky offset means it will be loaded in
memory with the wrong alignment:

(XEN) livepatch.c:425: livepatch: xen_bye_world: Loaded .livepatch.depends at 
000a08043

And later we get:
(XEN) livepatch.c:501: livepatch: xen_bye_world: .livepatch.depends is not 
aligned properly!

This fix forces all the test-cases to be built with a
.livepatch.depends structure containing the build-id extracted from
the hypervisor (except the xen_bye_world test-case).

We use the 'mkhex' tool instead of 'xxd' as the end result is an 'unsigned'
instead of 'char' type array - which naturally forces the alignment to be of 
four.
Also the 'mkhex' tools allows us to pass the section name as parameter.

The end result is much better alignment:

   [ 7] .livepatch.depend PROGBITS        00000000 000094 000024 00   A  0   0  
4

Note that thanks to 'unsigned int .. __note_depends' the symbol becomes
global:

$ readelf --symbols *.livepatch | grep depen
     23: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     6 note_depends
     49: 0000000000000000    36 OBJECT  GLOBAL HIDDEN    17 note_depends
     16: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     3 note_depends
     21: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     6 note_depends

See patch titled: "livepatch/arm/x86: Strip note_depends symbol from 
test-cases."
which fixes this.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

---
v2: First version
---
  docs/misc/livepatch.markdown           |  2 ++
  xen/test/livepatch/Makefile            | 56 +++++++++++++++-------------------
  xen/test/livepatch/xen_bye_world.c     |  1 +
  xen/test/livepatch/xen_hello_world.c   |  1 +
  xen/test/livepatch/xen_nop.c           |  1 +
  xen/test/livepatch/xen_replace_world.c |  1 +
  6 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 505dc37cda..922a64436f 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -430,6 +430,8 @@ checksum, MD5 checksum or any unique value.
The size of these structures varies with the --build-id linker option. +On ARM32 this section must by four-byte aligned.
+
  ## Hypercalls
We will employ the sub operations of the system management hypercall (sysctl).
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 6831383db1..89ad89dfd5 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -1,15 +1,7 @@
  include $(XEN_ROOT)/Config.mk
-ifeq ($(XEN_TARGET_ARCH),x86_64)
-OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
-endif
-ifeq ($(XEN_TARGET_ARCH),arm64)
-OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
-endif
-ifeq ($(XEN_TARGET_ARCH),arm32)
-OBJCOPY_MAGIC := -I binary -O elf32-littlearm -B arm
-endif
-
+NOTE_SYMBOL = "note_depends"
+NOTE_DEPENDS = "const  __section(\".livepatch.depends\") $(NOTE_SYMBOL)"
  CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}')
  CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
@@ -38,7 +30,7 @@ uninstall: .PHONY: clean
  clean::
-       rm -f *.o .*.o.d *.livepatch config.h
+       rm -f *.o .*.o.d *.livepatch config.h livepatch_depends.h 
hello_world_livepatch_depends.h *.bin
#
  # To compute these values we need the binary files: xen-syms
@@ -56,10 +48,10 @@ config.h: xen_hello_world_func.o
         echo "#define MINOR_VERSION_ADDR $(MINOR_VERSION_ADDR)"; \
         echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@
-xen_hello_world.o: config.h
+xen_hello_world.o: config.h livepatch_depends.h
.PHONY: $(LIVEPATCH)
-$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
+$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
        $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
#
@@ -71,40 +63,42 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o 
note.o
  # not be built (it is for EFI builds), and that we do not have
  # the note.o.bin to muck with (as it gets deleted)
  #
-.PHONY: note.o
-note.o:
-       $(OBJCOPY) -O binary --only-section=.note.gnu.build-id 
$(BASEDIR)/xen-syms $@.bin
-       $(OBJCOPY) $(OBJCOPY_MAGIC) \
-                  
--rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S 
$@.bin $@
-       rm -f $@.bin
+.PHONY: note.bin
+note.bin:
+       $(OBJCOPY) -O binary --only-section=.note.gnu.build-id 
$(BASEDIR)/xen-syms $@
+
+.PHONY: livepatch_depends.h
+livepatch_depends.h: note.bin
+       $(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > 
$@))

It looks quite odd to use a file in firmware/hvmloader for livepatch. Would it be possible to move mkhex to a generic place?

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.