[PATCH v1 0/4] Collection of livepatch-build-tools tweaks

[PATCH v1 0/4] Collection of livepatch-build-tools tweaks

[Mihails Strasuns]

Small series of unrelated patches currently used by AWS fork of
livepatch-build-tools but not present upstream. Symbol alias patch is only
relevant for the case of stacking multiple patches and thus may be of no use to
xen in general but still submitting it for extra visibility.

Michael Kurth (1):
  common.h: Flush stdout before writing to stderr

Raphael Ning (2):
  livepatch-build: Allow a patch to introduce new subdirs
  livepatch-gcc: Ignore buildid.o

Stanislav Uschakow / Mihails Strasuns (1):
  create-diff-object: Add new symbols for relocation aliases

 common.h             |   1 +
 create-diff-object.c | 132 +++++++++++++++++++++++++++++++++++++++++++
 livepatch-build      |   1 +
 livepatch-gcc        |   1 +
 4 files changed, 135 insertions(+)

-- 
2.38.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

[PATCH v1 1/4] common.h: Flush stdout before writing to stderr

[Mihails Strasuns]

Flush existing debug messages before writing an error to stderr.  stderr
is usually unbuffered and stdout is usually buffered. This results in
odd looking output when an error occurs and both stderr/stdout are
printed on the same console/file. More precisely, the error message is
printed in the middle of previously emitted debug messages.

Signed-off-by: Michael Kurth <mku@amazon.com>
---
 common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/common.h b/common.h
index 02c9b7b..9a9da79 100644
--- a/common.h
+++ b/common.h
@@ -10,6 +10,7 @@ extern char *childobj;
 
 #define DIFF_FATAL(format, ...) \
 ({ \
+	fflush(stdout); \
 	fprintf(stderr, "ERROR: %s: " format "\n", childobj, ##__VA_ARGS__); \
 	error(2, 0, "unreconcilable difference"); \
 })
-- 
2.38.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v1 1/4] common.h: Flush stdout before writing to stderr

[Ross Lagerwall]

> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> on behalf of Mihails Strasuns <mstrasun@amazon.com>
> Sent: Thursday, January 19, 2023 10:13 AM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: mstrasun@amazon.com <mstrasun@amazon.com>; Michael Kurth <mku@amazon.com>
> Subject: [PATCH v1 1/4] common.h: Flush stdout before writing to stderr 
>  
> Flush existing debug messages before writing an error to stderr.  stderr
> is usually unbuffered and stdout is usually buffered. This results in
> odd looking output when an error occurs and both stderr/stdout are
> printed on the same console/file. More precisely, the error message is
> printed in the middle of previously emitted debug messages.
> 
> Signed-off-by: Michael Kurth <mku@amazon.com>
> ---
>  common.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/common.h b/common.h
> index 02c9b7b..9a9da79 100644
> --- a/common.h
> +++ b/common.h
> @@ -10,6 +10,7 @@ extern char *childobj;
>  
>  #define DIFF_FATAL(format, ...) \
>  ({ \
> +       fflush(stdout); \
>          fprintf(stderr, "ERROR: %s: " format "\n", childobj, ##__VA_ARGS__); \
>          error(2, 0, "unreconcilable difference"); \
>  })
> -- 
> 2.38.1

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

[PATCH v1 2/4] livepatch-build: Allow a patch to introduce new subdirs

[Mihails Strasuns]

From: Raphael Ning <raphning@amazon.com>

Fix a bug in create_patch() where cp, strip, etc. will fail if the new
object file introduced by the patch is located in a new subdirectory:

 DEBUG: cp: cannot create regular file `output/xen/common/lu/lu.o': No such file or directory
 DEBUG: strip: 'output/xen/common/lu/lu.o': No such file

In this example, xen/common/lu/ does not exist in the original
(unpatched) Xen source tree. It needs to be created in output/ as well.

Signed-off-by: Raphael Ning <raphning@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
---
 livepatch-build | 1 +
 1 file changed, 1 insertion(+)

diff --git a/livepatch-build b/livepatch-build
index f7d6471..444daa9 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -232,6 +232,7 @@ function create_patch()
 
     NEW_FILES=$(comm -23 <(cd patched/xen && find . -type f -name '*.o' | sort) <(cd original/xen && find . -type f -name '*.o' | sort))
     for i in $NEW_FILES; do
+        mkdir -p "output/$(dirname "$i")"
         cp "patched/$i" "output/$i"
         [[ $STRIP -eq 1 ]] && strip --strip-unneeded "output/$i"
         CHANGED=1
-- 
2.38.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v1 2/4] livepatch-build: Allow a patch to introduce new subdirs

[Ross Lagerwall]

> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> on behalf of Mihails Strasuns <mstrasun@amazon.com>
> Sent: Thursday, January 19, 2023 10:13 AM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: mstrasun@amazon.com <mstrasun@amazon.com>; Raphael Ning <raphning@amazon.com>; Bjoern Doebel <doebel@amazon.de>; Martin Pohlack <mpohlack@amazon.de>
> Subject: [PATCH v1 2/4] livepatch-build: Allow a patch to introduce new subdirs 
>  
> From: Raphael Ning <raphning@amazon.com>
> 
> Fix a bug in create_patch() where cp, strip, etc. will fail if the new
> object file introduced by the patch is located in a new subdirectory:
> 
>  DEBUG: cp: cannot create regular file `output/xen/common/lu/lu.o': No such file or directory
>  DEBUG: strip: 'output/xen/common/lu/lu.o': No such file
> 
> In this example, xen/common/lu/ does not exist in the original
> (unpatched) Xen source tree. It needs to be created in output/ as well.
> 
> Signed-off-by: Raphael Ning <raphning@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> ---
>  livepatch-build | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/livepatch-build b/livepatch-build
> index f7d6471..444daa9 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -232,6 +232,7 @@ function create_patch()
>  
>      NEW_FILES=$(comm -23 <(cd patched/xen && find . -type f -name '*.o' | sort) <(cd original/xen && find . -type f -name '*.o' | sort))
>      for i in $NEW_FILES; do
> +        mkdir -p "output/$(dirname "$i")"
>          cp "patched/$i" "output/$i"
>          [[ $STRIP -eq 1 ]] && strip --strip-unneeded "output/$i"
>          CHANGED=1

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

[PATCH v1 3/4] livepatch-gcc: Ignore buildid.o

[Mihails Strasuns]

From: Raphael Ning <raphning@amazon.com>

Not all .o files generated by the Xen build need to be passed to
create-diff-object for analysis. The latest example is:

 Run create-diff-object on xen/arch/x86/efi/buildid.o
 Open base
 /usr/libexec/livepatch-build-tools/create-diff-object: ERROR: buildid.o: kpatch_create_section_list: 77: elf_getshdrnum

This file is special, as it does not contain any sections. It is
generated by objcopy from a magic string of bytes (see Xen commit
eee5909e9d1e x86/EFI: use less crude a way of generating the build ID),
which probably will never change. Therefore, livepatch-gcc should not
copy it to the output directory.

Signed-off-by: Raphael Ning <raphning@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>

CR: https://code.amazon.com/reviews/CR-63147772
---
 livepatch-gcc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/livepatch-gcc b/livepatch-gcc
index 91333d5..e72cc8d 100755
--- a/livepatch-gcc
+++ b/livepatch-gcc
@@ -65,6 +65,7 @@ elif [[ "$TOOLCHAINCMD" =~ $OBJCOPY_RE ]] ; then
         version.o|\
         debug.o|\
         efi/check.o|\
+        buildid.o|\
         .*.o)
             ;;
         *.o)
-- 
2.38.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v1 3/4] livepatch-gcc: Ignore buildid.o

[Ross Lagerwall]

> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> on behalf of Mihails Strasuns <mstrasun@amazon.com>
> Sent: Thursday, January 19, 2023 10:13 AM
> To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org>
> Cc: mstrasun@amazon.com <mstrasun@amazon.com>; Raphael Ning <raphning@amazon.com>; Bjoern Doebel <doebel@amazon.de>; Martin Pohlack <mpohlack@amazon.de>
> Subject: [PATCH v1 3/4] livepatch-gcc: Ignore buildid.o 
>  
> From: Raphael Ning <raphning@amazon.com>
> 
> Not all .o files generated by the Xen build need to be passed to
> create-diff-object for analysis. The latest example is:
> 
>  Run create-diff-object on xen/arch/x86/efi/buildid.o
>  Open base
>  /usr/libexec/livepatch-build-tools/create-diff-object: ERROR: buildid.o: kpatch_create_section_list: 77: elf_getshdrnum
> 
> This file is special, as it does not contain any sections. It is
> generated by objcopy from a magic string of bytes (see Xen commit
> eee5909e9d1e x86/EFI: use less crude a way of generating the build ID),
> which probably will never change. Therefore, livepatch-gcc should not
> copy it to the output directory.
> 
> Signed-off-by: Raphael Ning <raphning@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> 

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

[PATCH v1 4/4] create-diff-object: Add new symbols for relocation aliases

[Mihails Strasuns]

From: Stanislav Uschakow <suschako@amazon.com>

This fixes an issue when stacking multiple hotpatches modifying the same
function when there is also an internal call to that function within
these patches.

Two hotpatches touching the same subset of functions can result in a
situation where the second hotpatch is loaded but the code is never
executed. This happens when the symbol resolution of the first hotpatch
calculates the target address for the relocation entries based on the
hotpach instead of the original function. The resulting address for the
relocation-target will become the patched function instead of the
original one (A' -> B' instead of A' -> B).  In this case the trampoline
in the original function is not used. Patching this function again (B'')
re-sets the trampoline to the newly patched method. Since callq points
to the first patched function it never reaches the second version.

    2nd patch |          B''
    1st patch |  A'  ->  B'
    original  |  A   ->  B

This fix provides a solution for this problem by resolving the symbols
during patch generation using xen-syms. The patch iterates over all
relocation entries, and creates duplicates of all referenced symbols
with the scope set to SHN_ABS and the address to the one resolved from
xen-syms. This new symbol is prefixed with a unique string to generate a
new name.

This forces all calls to be directed to the original function which
contains the trampoline.

Signed-off-by: Stanislav Uschakow <suschako@amazon.com>
Signed-off-by: Mihails Strasuns <mstrasun@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
---
 create-diff-object.c | 132 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 132 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index a516670..808b288 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1878,6 +1878,25 @@ static char *mangle_local_symbol(char *filename, char *symname)
 	return s;
 }
 
+/*
+ * For a given mangled symbol name allocates a copy with the filename and the
+ * unmangled symbol separated by a null terminator.
+ */
+static bool unmangle_local_symbol(const char *mangled, char **filename,
+								  char **symname)
+{
+	if (filename == NULL || symname == NULL)
+		ERROR("Malformed output arguments passed to 'unmangle_local_symbol'");
+	char *sep = strchr(mangled, '#');
+	if (!sep)
+		return false;
+	int i = sep - mangled;
+	*filename = strdup(mangled);
+	(*filename)[i] = '\0';
+	*symname = *filename + i + 1;
+	return true;
+}
+
 /*
  * Rename local symbols to the filename#symbol format used by Xen's "special"
  * symbol table.
@@ -2290,6 +2309,116 @@ static error_t parse_opt (int key, char *arg, struct argp_state *state)
 	return 0;
 }
 
+/*
+ * For a given elf output target add a new entry to the symbol table
+ * which duplicates the provided source symbol.
+ */
+static struct symbol *insert_symbol_copy(struct kpatch_elf *elf,
+										 struct symbol *src, int index,
+										 char *new_name)
+{
+	struct symbol *new_sym;
+	ALLOC_LINK(new_sym, &elf->symbols);
+	new_sym->index = index;
+	new_sym->twin = NULL;
+	new_sym->sec = NULL;
+	new_sym->bind = src->bind;
+	new_sym->type = src->type;
+	new_sym->status = NEW;
+	new_sym->strip = 0;
+	new_sym->name = new_name;
+	/* The copy here will keep referring to the original symbol
+	   name which is fine - it will be updated later during the final elf
+	   generation. */
+	new_sym->sym = src->sym;
+	return new_sym;
+}
+
+/*
+ * In certain cases when 2 hotpatches are loaded patching the same subset of
+ * functions calling each other, the topmost hotpatch can become unreachable.
+ * This is caused by the loader when resolving symbols. If a symbol is both
+ * to be relocated and loaded (foo calls bar and both are patched to foo' and
+ * bar'), the callq target in foo' is set to bar' instead of the original bar.
+ * Patching bar again (bar'') cannot be reached from foo' because foo' does not
+ * use the trampoline in bar.
+ *
+ * This post-processing function fixes the problem by updating all
+ * non-livepatch relocation entries (i.e. originally created by a compiler) to
+ * be based on a special SHN_ABS with a fixed value resolved from xen-syms.
+ * These new symbols are not used by livepatch loader itself and only exist as
+ * a named constant for the relevant function address in the host process.
+ *
+ *	All new symbols are prefixed with a scalpel-specific prefix string. As they
+ *  are not used outside of a specific livepatch linking/relocation process,
+ *  the prefix can be the same across multiple livepatches.
+ */
+void add_absolute_symbol_aliases(struct kpatch_elf *elf,
+								 struct lookup_table *lookup)
+{
+	const char *prefix = "__livepatch_unique_";
+
+	int sym_count = 0;
+	struct symbol *sym;
+	list_for_each_entry (sym, &elf->symbols, list)
+		sym_count++;
+
+	struct section *sec;
+	list_for_each_entry (sec, &elf->sections, list) {
+		if (!is_rela_section(sec))
+			continue;
+		/* Ignore any relocation sections that are not related to a symbol
+		   text sections: */
+		if (strncmp(sec->name, ".rela.text.", 11) != 0)
+			continue;
+
+		struct rela *rel;
+		list_for_each_entry (rel, &sec->relas, list) {
+			if ((rel->sym->type != STT_FUNC) &&
+				(rel->sym->type != STT_OBJECT) &&
+				(rel->sym->type != STT_NOTYPE))
+				continue;
+			if (strlen(rel->sym->name) == 0)
+				ERROR("Unnamed symbol used in relocation");
+
+			struct lookup_result res = {0};
+			bool found = false;
+			/* Static/local symbols are mangled to include a filename prefix,
+			   thus need a special treatment for xen-syms lookup: */
+			if (rel->sym->bind == STB_LOCAL) {
+				char *filename, *symname;
+				if (!unmangle_local_symbol(rel->sym->name, &filename, &symname))
+					continue;
+				found = !lookup_local_symbol(lookup, symname, filename, &res);
+			} else
+				found = !lookup_global_symbol(lookup, rel->sym->name, &res);
+
+			if (found) {
+				char *buf = malloc(strlen(prefix) + strlen(rel->sym->name) + 1);
+				if (!buf)
+					ERROR("Failed to allocate buffer\n");
+				strcpy(buf, prefix);
+				strcpy(buf + strlen(prefix), rel->sym->name);
+
+				struct symbol *new_sym =
+					insert_symbol_copy(elf, rel->sym, sym_count++, buf);
+				/* Override key fields after copying. GLOBAL/LOCAL binding
+				   most likely doesn't matter here but forcing GLOBAL in case
+				   linker tries to do something smart: */
+				new_sym->bind = STB_GLOBAL;
+				new_sym->sym.st_value = res.value;
+				new_sym->sym.st_shndx = SHN_ABS;
+				new_sym->sym.st_info = GELF_ST_INFO(new_sym->bind, new_sym->type);
+ 
+				/* TODO: translate X86_64_PC32 rel type to X86_64_64 and
+				   validate that the relocation is still correvtly computed. */
+				log_debug("Added '%s' alias for '%s'\n", buf,  rel->sym->name);
+				rel->sym = new_sym;
+			}
+		}
+	}
+}
+
 static struct argp argp = { options, parse_opt, args_doc, 0 };
 
 int main(int argc, char *argv[])
@@ -2434,6 +2563,9 @@ int main(int argc, char *argv[])
 	log_debug("Rename local symbols\n");
 	livepatch_rename_local_symbols(kelf_out, hint);
 
+	log_debug("Add absolute symbol aliases for internal calls\n");
+	add_absolute_symbol_aliases(kelf_out, lookup);
+
 	/*
 	 *  At this point, the set of output sections and symbols is
 	 *  finalized.  Reorder the symbols into linker-compliant
-- 
2.38.1




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879