[PATCH 0/2] build-id adjustments

[PATCH 0/2] build-id adjustments

[Jan Beulich]

1: build-id: fix minor quirks
2: x86/EFI: use less crude way of generating the build ID

Signed-off-by: Jan Beulich <jbeulich@suse.com>


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

[PATCH 1/2] build-id: fix minor quirks

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1380 bytes --]

The initial size check in xen_build_id_check() came too late (after the
first access to the structure), but was mostly redundant with checks
done in all callers; convert it to a properly placed ASSERT(). The
"mostly" part being addressed too: xen_build_init() was off by one.

And then there was a stray semicolon.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -1,6 +1,7 @@
 #include <xen/compile.h>
 #include <xen/init.h>
 #include <xen/errno.h>
+#include <xen/lib.h>
 #include <xen/string.h>
 #include <xen/types.h>
 #include <xen/elf.h>
@@ -90,12 +91,11 @@ int xen_build_id_check(const Elf_Note *n
                        const void **p, unsigned int *len)
 {
     /* Check if we really have a build-id. */
+    ASSERT(n_sz > sizeof(*n));
+
     if ( NT_GNU_BUILD_ID != n->type )
         return -ENODATA;
 
-    if ( n_sz <= sizeof(*n) )
-        return -EINVAL;
-
     if ( n->namesz + n->descsz < n->namesz )
         return -EINVAL;
 
@@ -127,8 +127,8 @@ static int __init xen_build_init(void)
         return -ENODATA;
 
     /* Check for full Note header. */
-    if ( &n[1] > __note_gnu_build_id_end )
-        return -ENODATA;;
+    if ( &n[1] >= __note_gnu_build_id_end )
+        return -ENODATA;
 
     sz = (void *)__note_gnu_build_id_end - (void *)n;
 




[-- Attachment #2: build-id-check.patch --]
[-- Type: text/plain, Size: 1404 bytes --]

build-id: fix minor quirks

The initial size check in xen_build_id_check() came too late (after the
first access to the structure), but was mostly redundant with checks
done in all callers; convert it to a properly placed ASSERT(). The
"mostly" part being addressed too: xen_build_init() was off by one.

And then there was a stray semicolon.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -1,6 +1,7 @@
 #include <xen/compile.h>
 #include <xen/init.h>
 #include <xen/errno.h>
+#include <xen/lib.h>
 #include <xen/string.h>
 #include <xen/types.h>
 #include <xen/elf.h>
@@ -90,12 +91,11 @@ int xen_build_id_check(const Elf_Note *n
                        const void **p, unsigned int *len)
 {
     /* Check if we really have a build-id. */
+    ASSERT(n_sz > sizeof(*n));
+
     if ( NT_GNU_BUILD_ID != n->type )
         return -ENODATA;
 
-    if ( n_sz <= sizeof(*n) )
-        return -EINVAL;
-
     if ( n->namesz + n->descsz < n->namesz )
         return -EINVAL;
 
@@ -127,8 +127,8 @@ static int __init xen_build_init(void)
         return -ENODATA;
 
     /* Check for full Note header. */
-    if ( &n[1] > __note_gnu_build_id_end )
-        return -ENODATA;;
+    if ( &n[1] >= __note_gnu_build_id_end )
+        return -ENODATA;
 
     sz = (void *)__note_gnu_build_id_end - (void *)n;
 

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

[PATCH 2/2] x86/EFI: use less crude way of generating the build ID

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 8479 bytes --]

Recent enough binutils support --build-id also for COFF/PE output, and
hence we should use that in favor of the original hack when possible.

This gets complicated by the linker requiring at least one COFF object
file to attach the .buildid section to. Hence the patch introduces a
buildid.ihex (in order to avoid introducing binary files into the repo)
which then gets converted to a binary minimal COFF object (no sections,
no symbols).

Also (to avoid both code fragment going out of sync) remove an unneeded
ALIGN() from xen.lds.S: Adding an equivalent of it to the .buildid
section would cause the _erodata symbol to become associated with the
wrong section again (see commit 0970299de5 ["x86/EFI + Live Patch:
avoid symbol address truncation"]). And it's pointless because the
alignment already gets properly set by the input section(s).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Question of course is whether consumers of the build ID other than Xen
itself need to be taught how to find it in an EFI binary.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -158,24 +158,30 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(N
 $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
 # Don't use $(wildcard ...) here - at least make 3.80 expands this too early!
 $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:)
+
 ifneq ($(build_id_linker),)
-$(TARGET).efi: note.o
+ifeq ($(call ld-ver-build-id,$(LD) $(EFI_LDFLAGS)),y)
+CFLAGS += -DBUILD_ID_EFI
+EFI_LDFLAGS += $(build_id_linker)
+note_file := efi/buildid.o
+else
 note_file := note.o
+endif
 else
 note_file :=
 endif
 
-$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
+$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
-	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
+	                $(BASEDIR)/common/symbols-dummy.o $(note_file) -o $(@D)/.$(@F).$(base).0 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
 		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
-	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) :
+	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file) -o $(@D)/.$(@F).$(base).1 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
 		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S
@@ -185,8 +191,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi
 	if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi
 	rm -f $(@D)/.$(@F).[0-9]*
 
-efi/boot.init.o efi/runtime.o efi/compat.o: $(BASEDIR)/arch/x86/efi/built_in.o
-efi/boot.init.o efi/runtime.o efi/compat.o: ;
+efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
+efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
 	$(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $<
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
 efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
 efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
 
-extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
+extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
+
+%.o: %.ihex
+	$(OBJCOPY) -I ihex -O binary $< $@
 
 stub.o: $(extra-y)
--- /dev/null
+++ b/xen/arch/x86/efi/buildid.ihex
@@ -0,0 +1,3 @@
+:10000000648600004D8DAD57140000000000000014
+:0400100000000000EC
+:00000001FF
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -342,6 +342,7 @@ int main(int argc, char *argv[])
          */
         if ( memcmp(sec1[i].name, ".initcal", sizeof(sec1[i].name)) == 0 ||
              memcmp(sec1[i].name, ".init.se", sizeof(sec1[i].name)) == 0 ||
+             memcmp(sec1[i].name, ".buildid", sizeof(sec1[i].name)) == 0 ||
              memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 )
             continue;
 
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -91,7 +91,7 @@ SECTIONS
        *(.rodata)
        *(.rodata.*)
 
-#if defined(BUILD_ID) && defined(EFI)
+#if defined(BUILD_ID) && defined(EFI) && !defined(BUILD_ID_EFI)
 /*
  * No mechanism to put an PT_NOTE in the EFI file - so put
  * it in .rodata section. (notes.o supplies us with .note.gnu.build-id).
@@ -120,19 +120,26 @@ SECTIONS
 #endif
   } :text
 
-#if defined(BUILD_ID) && !defined(EFI)
+#if defined(BUILD_ID)
+#if !defined(EFI)
 /*
  * What a strange section name. The reason is that on ELF builds this section
  * is extracted to notes.o (which then is ingested in the EFI file). But the
  * compiler may want to inject other things in the .note which we don't care
  * about - hence this unique name.
  */
-  . = ALIGN(4);
   .note.gnu.build-id : {
        __note_gnu_build_id_start = .;
        *(.note.gnu.build-id)
        __note_gnu_build_id_end = .;
   } :note :text
+#elif defined(BUILD_ID_EFI)
+  .buildid : {
+       __note_gnu_build_id_start = .;
+       *(.buildid)
+       __note_gnu_build_id_end = .;
+  } :text
+#endif
 #endif
   _erodata = .;
 
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -4,6 +4,7 @@
 #include <xen/lib.h>
 #include <xen/string.h>
 #include <xen/types.h>
+#include <xen/efi.h>
 #include <xen/elf.h>
 #include <xen/version.h>
 
@@ -117,10 +118,33 @@ int xen_build_id_check(const Elf_Note *n
     return 0;
 }
 
+struct pe_external_debug_directory
+{
+    uint32_t characteristics;
+    uint32_t time_stamp;
+    uint16_t major_version;
+    uint16_t minor_version;
+#define PE_IMAGE_DEBUG_TYPE_CODEVIEW 2
+    uint32_t type;
+    uint32_t size;
+    uint32_t rva_of_data;
+    uint32_t filepos_of_data;
+};
+
+struct cv_info_pdb70
+{
+#define CVINFO_PDB70_CVSIGNATURE 0x53445352 /* "RSDS" */
+    uint32_t cv_signature;
+    unsigned char signature[16];
+    uint32_t age;
+    char pdb_filename[];
+};
+
 static int __init xen_build_init(void)
 {
     const Elf_Note *n = __note_gnu_build_id_start;
     unsigned int sz;
+    int rc;
 
     /* --build-id invoked with wrong parameters. */
     if ( __note_gnu_build_id_end <= &n[0] )
@@ -132,7 +156,38 @@ static int __init xen_build_init(void)
 
     sz = (void *)__note_gnu_build_id_end - (void *)n;
 
-    return xen_build_id_check(n, sz, &build_id_p, &build_id_len);
+    rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len);
+
+#ifdef CONFIG_X86
+    /* Alternatively we may have a CodeView record from an EFI build. */
+    if ( rc && efi_enabled )
+    {
+        const struct pe_external_debug_directory *dir = (const void *)n;
+
+        /*
+         * Validate that the full-note-header check above won't prevent
+         * fall-through to the CodeView case here.
+         */
+        BUILD_BUG_ON(sizeof(*n) > sizeof(*dir));
+
+        if ( sz > sizeof(*dir) + sizeof(struct cv_info_pdb70) &&
+             dir->type == PE_IMAGE_DEBUG_TYPE_CODEVIEW &&
+             dir->size > sizeof(struct cv_info_pdb70) &&
+             XEN_VIRT_START + dir->rva_of_data == (unsigned long)(dir + 1) )
+        {
+            const struct cv_info_pdb70 *info = (const void *)(dir + 1);
+
+            if ( info->cv_signature == CVINFO_PDB70_CVSIGNATURE )
+            {
+                build_id_p = info->signature;
+                build_id_len = sizeof(info->signature);
+                rc = 0;
+            }
+        }
+    }
+#endif
+
+    return rc;
 }
 __initcall(xen_build_init);
 #endif



[-- Attachment #2: x86-EFI-build-ID.patch --]
[-- Type: text/plain, Size: 8533 bytes --]

x86/EFI: use less crude way of generating the build ID

Recent enough binutils support --build-id also for COFF/PE output, and
hence we should use that in favor of the original hack when possible.

This gets complicated by the linker requiring at least one COFF object
file to attach the .buildid section to. Hence the patch introduces a
buildid.ihex (in order to avoid introducing binary files into the repo)
which then gets converted to a binary minimal COFF object (no sections,
no symbols).

Also (to avoid both code fragment going out of sync) remove an unneeded
ALIGN() from xen.lds.S: Adding an equivalent of it to the .buildid
section would cause the _erodata symbol to become associated with the
wrong section again (see commit 0970299de5 ["x86/EFI + Live Patch:
avoid symbol address truncation"]). And it's pointless because the
alignment already gets properly set by the input section(s).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Question of course is whether consumers of the build ID other than Xen
itself need to be taught how to find it in an EFI binary.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -158,24 +158,30 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(N
 $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
 # Don't use $(wildcard ...) here - at least make 3.80 expands this too early!
 $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:)
+
 ifneq ($(build_id_linker),)
-$(TARGET).efi: note.o
+ifeq ($(call ld-ver-build-id,$(LD) $(EFI_LDFLAGS)),y)
+CFLAGS += -DBUILD_ID_EFI
+EFI_LDFLAGS += $(build_id_linker)
+note_file := efi/buildid.o
+else
 note_file := note.o
+endif
 else
 note_file :=
 endif
 
-$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
+$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
-	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
+	                $(BASEDIR)/common/symbols-dummy.o $(note_file) -o $(@D)/.$(@F).$(base).0 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
 		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
-	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) :
+	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file) -o $(@D)/.$(@F).$(base).1 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
 		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S
@@ -185,8 +191,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi
 	if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi
 	rm -f $(@D)/.$(@F).[0-9]*
 
-efi/boot.init.o efi/runtime.o efi/compat.o: $(BASEDIR)/arch/x86/efi/built_in.o
-efi/boot.init.o efi/runtime.o efi/compat.o: ;
+efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
+efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
 	$(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $<
--- a/xen/arch/x86/efi/Makefile
+++ b/xen/arch/x86/efi/Makefile
@@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
 efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
 efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
 
-extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
+extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
+
+%.o: %.ihex
+	$(OBJCOPY) -I ihex -O binary $< $@
 
 stub.o: $(extra-y)
--- /dev/null
+++ b/xen/arch/x86/efi/buildid.ihex
@@ -0,0 +1,3 @@
+:10000000648600004D8DAD57140000000000000014
+:0400100000000000EC
+:00000001FF
--- a/xen/arch/x86/efi/mkreloc.c
+++ b/xen/arch/x86/efi/mkreloc.c
@@ -342,6 +342,7 @@ int main(int argc, char *argv[])
          */
         if ( memcmp(sec1[i].name, ".initcal", sizeof(sec1[i].name)) == 0 ||
              memcmp(sec1[i].name, ".init.se", sizeof(sec1[i].name)) == 0 ||
+             memcmp(sec1[i].name, ".buildid", sizeof(sec1[i].name)) == 0 ||
              memcmp(sec1[i].name, ".lockpro", sizeof(sec1[i].name)) == 0 )
             continue;
 
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -91,7 +91,7 @@ SECTIONS
        *(.rodata)
        *(.rodata.*)
 
-#if defined(BUILD_ID) && defined(EFI)
+#if defined(BUILD_ID) && defined(EFI) && !defined(BUILD_ID_EFI)
 /*
  * No mechanism to put an PT_NOTE in the EFI file - so put
  * it in .rodata section. (notes.o supplies us with .note.gnu.build-id).
@@ -120,19 +120,26 @@ SECTIONS
 #endif
   } :text
 
-#if defined(BUILD_ID) && !defined(EFI)
+#if defined(BUILD_ID)
+#if !defined(EFI)
 /*
  * What a strange section name. The reason is that on ELF builds this section
  * is extracted to notes.o (which then is ingested in the EFI file). But the
  * compiler may want to inject other things in the .note which we don't care
  * about - hence this unique name.
  */
-  . = ALIGN(4);
   .note.gnu.build-id : {
        __note_gnu_build_id_start = .;
        *(.note.gnu.build-id)
        __note_gnu_build_id_end = .;
   } :note :text
+#elif defined(BUILD_ID_EFI)
+  .buildid : {
+       __note_gnu_build_id_start = .;
+       *(.buildid)
+       __note_gnu_build_id_end = .;
+  } :text
+#endif
 #endif
   _erodata = .;
 
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -4,6 +4,7 @@
 #include <xen/lib.h>
 #include <xen/string.h>
 #include <xen/types.h>
+#include <xen/efi.h>
 #include <xen/elf.h>
 #include <xen/version.h>
 
@@ -117,10 +118,33 @@ int xen_build_id_check(const Elf_Note *n
     return 0;
 }
 
+struct pe_external_debug_directory
+{
+    uint32_t characteristics;
+    uint32_t time_stamp;
+    uint16_t major_version;
+    uint16_t minor_version;
+#define PE_IMAGE_DEBUG_TYPE_CODEVIEW 2
+    uint32_t type;
+    uint32_t size;
+    uint32_t rva_of_data;
+    uint32_t filepos_of_data;
+};
+
+struct cv_info_pdb70
+{
+#define CVINFO_PDB70_CVSIGNATURE 0x53445352 /* "RSDS" */
+    uint32_t cv_signature;
+    unsigned char signature[16];
+    uint32_t age;
+    char pdb_filename[];
+};
+
 static int __init xen_build_init(void)
 {
     const Elf_Note *n = __note_gnu_build_id_start;
     unsigned int sz;
+    int rc;
 
     /* --build-id invoked with wrong parameters. */
     if ( __note_gnu_build_id_end <= &n[0] )
@@ -132,7 +156,38 @@ static int __init xen_build_init(void)
 
     sz = (void *)__note_gnu_build_id_end - (void *)n;
 
-    return xen_build_id_check(n, sz, &build_id_p, &build_id_len);
+    rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len);
+
+#ifdef CONFIG_X86
+    /* Alternatively we may have a CodeView record from an EFI build. */
+    if ( rc && efi_enabled )
+    {
+        const struct pe_external_debug_directory *dir = (const void *)n;
+
+        /*
+         * Validate that the full-note-header check above won't prevent
+         * fall-through to the CodeView case here.
+         */
+        BUILD_BUG_ON(sizeof(*n) > sizeof(*dir));
+
+        if ( sz > sizeof(*dir) + sizeof(struct cv_info_pdb70) &&
+             dir->type == PE_IMAGE_DEBUG_TYPE_CODEVIEW &&
+             dir->size > sizeof(struct cv_info_pdb70) &&
+             XEN_VIRT_START + dir->rva_of_data == (unsigned long)(dir + 1) )
+        {
+            const struct cv_info_pdb70 *info = (const void *)(dir + 1);
+
+            if ( info->cv_signature == CVINFO_PDB70_CVSIGNATURE )
+            {
+                build_id_p = info->signature;
+                build_id_len = sizeof(info->signature);
+                rc = 0;
+            }
+        }
+    }
+#endif
+
+    return rc;
 }
 __initcall(xen_build_init);
 #endif

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH 1/2] build-id: fix minor quirks

[Konrad Rzeszutek Wilk]

On Fri, Aug 12, 2016 at 08:45:02AM -0600, Jan Beulich wrote:
> The initial size check in xen_build_id_check() came too late (after the
> first access to the structure), but was mostly redundant with checks
> done in all callers; convert it to a properly placed ASSERT(). The
> "mostly" part being addressed too: xen_build_init() was off by one.
> 
> And then there was a stray semicolon.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

Re: [PATCH 2/2] x86/EFI: use less crude way of generating the build ID

[Konrad Rzeszutek Wilk]

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -158,24 +158,30 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(N
>  $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
>  # Don't use $(wildcard ...) here - at least make 3.80 expands this too early!
>  $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:)
> +
>  ifneq ($(build_id_linker),)
> -$(TARGET).efi: note.o
> +ifeq ($(call ld-ver-build-id,$(LD) $(EFI_LDFLAGS)),y)
> +CFLAGS += -DBUILD_ID_EFI
> +EFI_LDFLAGS += $(build_id_linker)
> +note_file := efi/buildid.o
> +else
>  note_file := note.o
> +endif
>  else
>  note_file :=
>  endif
>  
> -$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
> +$(TARGET).efi: prelink-efi.o $(note_file) efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
>  	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
>  	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
> -	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
> +	                $(BASEDIR)/common/symbols-dummy.o $(note_file) -o $(@D)/.$(@F).$(base).0 &&) :
>  	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
>  	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
>  		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0s.S
>  	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
>  	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
>  	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
> -	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) :
> +	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o $(note_file) -o $(@D)/.$(@F).$(base).1 &&) :
>  	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
>  	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
>  		| $(guard) $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1s.S
> @@ -185,8 +191,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi
>  	if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi
>  	rm -f $(@D)/.$(@F).[0-9]*
>  
> -efi/boot.init.o efi/runtime.o efi/compat.o: $(BASEDIR)/arch/x86/efi/built_in.o
> -efi/boot.init.o efi/runtime.o efi/compat.o: ;
> +efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/efi/built_in.o
> +efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
>  
>  asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
>  	$(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $<
> --- a/xen/arch/x86/efi/Makefile
> +++ b/xen/arch/x86/efi/Makefile
> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
>  efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
>  
> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
> +
> +%.o: %.ihex
> +	$(OBJCOPY) -I ihex -O binary $< $@
>  



Under  Ubuntu 14.04.4 this fails compilation:


make[4]: *** No rule to make target `buildid.o', needed by `stub.o'.
Stop.


The properties of Ubuntu 14.04.4 are:
konrad@ubuntu:~/xen$ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
konrad@ubuntu:~/xen/xen$ ld --version
GNU ld (GNU Binutils for Ubuntu) 2.24
konrad@ubuntu:~/xen/xen$ ld -mi386pep
ld: no input files

konrad@ubuntu:~/xen/xen$ cat /etc/debian_version 
jessie/sid

Hadn't dug in this.

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

Re: [PATCH 2/2] x86/EFI: use less crude way of generating the build ID

[Jan Beulich]

>>> On 15.08.16 at 01:42, <konrad@kernel.org> wrote:
>> --- a/xen/arch/x86/efi/Makefile
>> +++ b/xen/arch/x86/efi/Makefile
>> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
>>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
>>  efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
>>  
>> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
>> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
>> +
>> +%.o: %.ihex
>> +	$(OBJCOPY) -I ihex -O binary $< $@
> 
> 
> Under  Ubuntu 14.04.4 this fails compilation:
> 
> 
> make[4]: *** No rule to make target `buildid.o', needed by `stub.o'.

That's extremely odd, namely considering that the rule is right
there in the quoted text above. Could you double check
xen/arch/x86/efi/buildid.ihex is actually there?

> The properties of Ubuntu 14.04.4 are:
> konrad@ubuntu:~/xen$ gcc --version
> gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
> konrad@ubuntu:~/xen/xen$ ld --version
> GNU ld (GNU Binutils for Ubuntu) 2.24
> konrad@ubuntu:~/xen/xen$ ld -mi386pep
> ld: no input files
> 
> konrad@ubuntu:~/xen/xen$ cat /etc/debian_version 
> jessie/sid

For the error above the version of make is actually the most
relevant one (albeit for this trivial a rule I can't really how the
make version could matter). Since you've chopped off other
make output - does the error indeed occur in xen/arch/x86/efi/
(and not in xen/arch/x86/)? I agree this should be the case,
since in the other directory the printed name would have to be
efi/buildid.o - I'm just trying make sure impossible things really
are impossible.

Jan


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

Re: [PATCH 2/2] x86/EFI: use less crude way of generating the build ID

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote:
> >>> On 15.08.16 at 01:42, <konrad@kernel.org> wrote:
> >> --- a/xen/arch/x86/efi/Makefile
> >> +++ b/xen/arch/x86/efi/Makefile
> >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
> >>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
> >>  efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
> >>  
> >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
> >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
> >> +
> >> +%.o: %.ihex
> >> +	$(OBJCOPY) -I ihex -O binary $< $@
> > 
> > 
> > Under  Ubuntu 14.04.4 this fails compilation:
> > 
> > 
> > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'.
> 
> That's extremely odd, namely considering that the rule is right
> there in the quoted text above. Could you double check
> xen/arch/x86/efi/buildid.ihex is actually there?

<sigh>
No it is not. I had issues with your email (git am -s) so I did it by
hand (patch -p2) and of course forgot to include the new file. And later
on built it on another OS after pulling from a git tree which missed this file.

Sorry about the false report!

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

Re: [PATCH 2/2] x86/EFI: use less crude way of generating the build ID

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2016 at 10:15:47AM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote:
> > >>> On 15.08.16 at 01:42, <konrad@kernel.org> wrote:
> > >> --- a/xen/arch/x86/efi/Makefile
> > >> +++ b/xen/arch/x86/efi/Makefile
> > >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
> > >>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi check.o 2>disabled && echo y))
> > >>  efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call create,boot.init.o); $(call create,runtime.o)))
> > >>  
> > >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
> > >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
> > >> +
> > >> +%.o: %.ihex
> > >> +	$(OBJCOPY) -I ihex -O binary $< $@
> > > 
> > > 
> > > Under  Ubuntu 14.04.4 this fails compilation:
> > > 
> > > 
> > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'.
> > 
> > That's extremely odd, namely considering that the rule is right
> > there in the quoted text above. Could you double check
> > xen/arch/x86/efi/buildid.ihex is actually there?
> 
> <sigh>
> No it is not. I had issues with your email (git am -s) so I did it by
> hand (patch -p2) and of course forgot to include the new file. And later
> on built it on another OS after pulling from a git tree which missed this file.
> 
> Sorry about the false report!

About the review:

> 
> x86/EFI: use less crude way of generating the build ID
> 
> Recent enough binutils support --build-id also for COFF/PE output, and
> hence we should use that in favor of the original hack when possible.

Could you mention the exact version that gained this?

> 
> This gets complicated by the linker requiring at least one COFF object
> file to attach the .buildid section to. Hence the patch introduces a

Is that requirement spelled out in the manpage of the linker?

> buildid.ihex (in order to avoid introducing binary files into the repo)
> which then gets converted to a binary minimal COFF object (no sections,
> no symbols).

Otherwise:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks!

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

Re: [PATCH 2/2] x86/EFI: use less crude way of generating the build ID

[Jan Beulich]

>>> On 19.08.16 at 18:14, <konrad.wilk@oracle.com> wrote:
> On Mon, Aug 15, 2016 at 10:15:47AM -0400, Konrad Rzeszutek Wilk wrote:
>> On Mon, Aug 15, 2016 at 01:58:47AM -0600, Jan Beulich wrote:
>> > >>> On 15.08.16 at 01:42, <konrad@kernel.org> wrote:
>> > >> --- a/xen/arch/x86/efi/Makefile
>> > >> +++ b/xen/arch/x86/efi/Makefile
>> > >> @@ -9,6 +9,9 @@ efi := $(if $(efi),$(shell $(CC) $(filte
>> > >>  efi := $(if $(efi),$(shell $(LD) -mi386pep --subsystem=10 -o check.efi 
> check.o 2>disabled && echo y))
>> > >>  efi := $(if $(efi),$(shell rm disabled)y,$(shell $(call 
> create,boot.init.o); $(call create,runtime.o)))
>> > >>  
>> > >> -extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o
>> > >> +extra-$(efi) += boot.init.o relocs-dummy.o runtime.o compat.o buildid.o
>> > >> +
>> > >> +%.o: %.ihex
>> > >> +	$(OBJCOPY) -I ihex -O binary $< $@
>> > > 
>> > > 
>> > > Under  Ubuntu 14.04.4 this fails compilation:
>> > > 
>> > > 
>> > > make[4]: *** No rule to make target `buildid.o', needed by `stub.o'.
>> > 
>> > That's extremely odd, namely considering that the rule is right
>> > there in the quoted text above. Could you double check
>> > xen/arch/x86/efi/buildid.ihex is actually there?
>> 
>> <sigh>
>> No it is not. I had issues with your email (git am -s) so I did it by
>> hand (patch -p2) and of course forgot to include the new file. And later
>> on built it on another OS after pulling from a git tree which missed this 
> file.
>> 
>> Sorry about the false report!
> 
> About the review:
> 
>> 
>> x86/EFI: use less crude way of generating the build ID
>> 
>> Recent enough binutils support --build-id also for COFF/PE output, and
>> hence we should use that in favor of the original hack when possible.
> 
> Could you mention the exact version that gained this?

Looks like that was 2.25; added.

>> This gets complicated by the linker requiring at least one COFF object
>> file to attach the .buildid section to. Hence the patch introduces a
> 
> Is that requirement spelled out in the manpage of the linker?

No. Since it mysteriously failed initially, I had to dig into their
sources to figure out that requirement.

>> buildid.ihex (in order to avoid introducing binary files into the repo)
>> which then gets converted to a binary minimal COFF object (no sections,
>> no symbols).
> 
> Otherwise:
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Thanks.

Jan


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