[livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[Pawel Wieczorkiewicz]

When there is no changed function in the generated payload, do not
create an empty .livepatch.funcs section. Hypervisor code considers
such payloads as broken and rejects to load them.

Such payloads without any changed functions may appear when only
hooks are specified.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Mazein <amazein@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>

CR: https://code.amazon.com/reviews/CR-7368634
---
 create-diff-object.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index 82f777e..af2245c 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1744,6 +1744,11 @@ static void livepatch_create_patches_sections(struct kpatch_elf *kelf,
 		if (sym->type == STT_FUNC && sym->status == CHANGED)
 			nr++;
 
+	if (nr == 0) {
+		log_debug("No changed functions found. Skipping .livepatch.funcs section creation\n");
+		return;
+	}
+
 	/* create text/rela section pair */
 	sec = create_section_pair(kelf, ".livepatch.funcs", sizeof(*funcs), nr);
 	relasec = sec->rela;
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



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

[livepatch-build-tools part3 2/3] create-diff-object: Extend patchability verification: STN_UNDEF

[Pawel Wieczorkiewicz]

During verification check if all sections do not contain any entries
with undefined symbols (STN_UNDEF). This situation can happen when a
section is copied over from its original object to a patched object,
but various symbols related to the section are not copied along.
This scenario happens typically during stacked hotpatches creation
(between 2 different hotpatch modules).

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>

CR: https://code.amazon.com/reviews/CR-7368645
---
 create-diff-object.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/create-diff-object.c b/create-diff-object.c
index af2245c..96b6716 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1448,6 +1448,31 @@ static void kpatch_print_changes(struct kpatch_elf *kelf)
 	}
 }
 
+static inline int get_section_entry_size(const struct section *sec, struct kpatch_elf *kelf)
+{
+	int entry_size;
+
+	/*
+	 * Base sections typically do not define fixed size elements.
+	 * Detect section's element size in case it's a special section.
+	 * Otherwise, skip it due to an unknown sh_entsize.
+	 */
+	entry_size = sec->sh.sh_entsize;
+	if (entry_size == 0) {
+		struct special_section *special;
+
+		/* Find special section group_size. */
+		for (special = special_sections; special->name; special++) {
+			/* Match sections starting with special->name prefix */
+			if (!strncmp(sec->name, special->name, strlen(special->name))) {
+				return special->group_size(kelf, 0);
+			}
+		}
+        }
+
+        return entry_size;
+}
+
 static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 {
 	struct section *sec;
@@ -1471,6 +1496,41 @@ static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 			errs++;
 		}
 
+		if (sec->include) {
+			if (!is_standard_section(sec) && !is_rela_section(sec) &&
+			    !is_debug_section(sec) && !is_special_section(sec)) {
+				if (!is_referenced_section(sec, kelf)) {
+					log_normal("section %s included, but not referenced\n", sec->name);
+					errs++;
+				}
+			}
+
+			/* Check if a RELA section does not contain any entries with
+			 * undefined symbols (STN_UNDEF). This situation can happen
+			 * when a section is copied over from its original object to
+			 * a patched object, but various symbols related to the section
+			 * are not copied along.
+			 */
+			if (is_rela_section(sec)) {
+				int offset, entry_size = get_section_entry_size(sec, kelf);
+				struct rela *rela;
+
+				for ( offset = 0; offset < sec->base->data->d_size && entry_size; offset += entry_size ) {
+					list_for_each_entry(rela, &sec->relas, list) {
+						if (rela->offset < offset || rela->offset >= offset + entry_size)
+							continue;
+
+						if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
+						    (!rela->sym->include && (rela->sym->status == SAME))) {
+							log_normal("section %s has an entry with a STN_UNDEF symbol: %s\n",
+								   sec->name, rela->sym->name ? rela->sym->name : "none");
+							errs++;
+						}
+					}
+				}
+			}
+		}
+
 		/*
 		 * ensure we aren't including .data.* or .bss.*
 		 * (.data.unlikely is ok b/c it only has __warned vars)
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



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

[livepatch-build-tools part3 3/3] create-diff-object: Strip all undefined entires of known size

[Pawel Wieczorkiewicz]

The patched ELF object file contains all sections and symbols as
resulted from the compilation. However, certain symbols may not be
copied over to the resulting object file, due to being unchanged or
not included for other reasons.
In such situation the resulting object file has the entire sections
copied along (with all their entries unchanged), while some of the
corresponding symbols are not copied along at all.
This leads to having incorrect dummy (STN_UNDEF) entries in the final
hotpatch ELF file.

The newly added function livepatch_strip_undefined_elements() detects
and removes all undefined RELA entries as well as their corresponding
PROGBITS section entries.
Since the sections may contain elements of unknown size (sh.sh_entsize
== 0), perform the strip only on sections with well defined entry
sizes.

After replacing the stripped rela list, it is assumed that the next
invocation of the kpatch_rebuild_rela_section_data() will adjust all
section header parameters according to the current state.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>

CR: https://code.amazon.com/reviews/CR-7368662
---
 create-diff-object.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 113 insertions(+), 2 deletions(-)

diff --git a/create-diff-object.c b/create-diff-object.c
index 96b6716..4f031ef 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1473,6 +1473,13 @@ static inline int get_section_entry_size(const struct section *sec, struct kpatc
         return entry_size;
 }
 
+/* Check if RELA entry has undefined (STN_UNDEF) or unchanged/not-included elements. */
+static inline bool has_rela_undefined_element(const struct rela *rela)
+{
+	return (GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
+	       (!rela->sym->include && (rela->sym->status == SAME));
+}
+
 static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 {
 	struct section *sec;
@@ -1520,8 +1527,7 @@ static void kpatch_verify_patchability(struct kpatch_elf *kelf)
 						if (rela->offset < offset || rela->offset >= offset + entry_size)
 							continue;
 
-						if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
-						    (!rela->sym->include && (rela->sym->status == SAME))) {
+						if (has_rela_undefined_element(rela)) {
 							log_normal("section %s has an entry with a STN_UNDEF symbol: %s\n",
 								   sec->name, rela->sym->name ? rela->sym->name : "none");
 							errs++;
@@ -1889,6 +1895,109 @@ static void livepatch_create_patches_sections(struct kpatch_elf *kelf,
 
 }
 
+/*
+ * The patched ELF object file contains all sections and symbols as resulted
+ * from the compilation. However, certain symbols may not be copied over to
+ * the resulting object file, due to being unchanged or not included for other
+ * reasons.
+ * In such situation the resulting object file has the entire sections copied
+ * along (with all their entries unchanged), while some of the corresponding
+ * symbols are not copied along at all.
+ * This leads to having incorrect dummy (STN_UNDEF) entries in the final
+ * hotpatch ELF file.
+ * This functions removes all undefined entries of known size from both
+ * RELA and PROGBITS sections of the patched elf object.
+ */
+static void livepatch_strip_undefined_elements(struct kpatch_elf *kelf)
+{
+	struct section *sec;
+
+	list_for_each_entry(sec, &kelf->sections, list) {
+		struct rela *rela, *safe;
+		int src_offset = 0, dst_offset = 0;
+		int entry_size, align, aligned_size;
+		char *src, *dst;
+		LIST_HEAD(newrelas);
+
+		/* use RELA section to find all its undefined entries */
+		if (!is_rela_section(sec))
+			continue;
+
+		/* only known, fixed-size entries can be stripped */
+		entry_size = get_section_entry_size(sec->base, kelf);
+		if (entry_size == 0)
+			continue;
+
+		/* alloc buffer for new base section */
+		dst = malloc(sec->base->sh.sh_size);
+		if (!dst)
+			ERROR("malloc");
+
+		/* iterate through all entries of a corresponding base section for this RELA section */
+		for ( src = sec->base->data->d_buf; src_offset < sec->base->sh.sh_size; src_offset += entry_size ) {
+			bool found_valid = false;
+
+			list_for_each_entry_safe(rela, safe, &sec->relas, list) {
+				/* check all RELA elements looking for corresponding entry references */
+				if (rela->offset < src_offset || rela->offset >= src_offset + entry_size)
+					continue;
+
+				/* ignore all undefined (STN_UNDEF) or unchanged/not-included elements */
+				if (has_rela_undefined_element(rela)) {
+					log_normal("Found a STN_UNDEF symbol %s in section %s\n",
+						   rela->sym->name, sec->name);
+					continue;
+				}
+
+				/*
+				 * A correct match has been found, so move it to a new list.
+				 * Original list will be destroyed along with the entire kelf
+				 * object, so the reference must be preserved.
+				 */
+				found_valid = true;
+				list_del(&rela->list);
+				list_add_tail(&rela->list, &newrelas);
+
+				rela->offset -= src_offset - dst_offset;
+				rela->rela.r_offset = rela->offset;
+			}
+
+			/* there is a valid RELA entry, so copy current entry */
+			if (found_valid) {
+				/* copy base section group */
+				memcpy(dst + dst_offset, src + src_offset, entry_size);
+				dst_offset += entry_size;
+			}
+		}
+
+		/* verify that entry_size is a divisor of aligned section size */
+		align = sec->base->sh.sh_addralign;
+		aligned_size = ((sec->base->sh.sh_size + align - 1) / align) * align;
+		if (src_offset != aligned_size)
+			ERROR("group size mismatch for section %s\n", sec->base->name);
+
+		if (!dst_offset) {
+			/* no changed or global functions referenced */
+			sec->status = sec->base->status = SAME;
+			sec->include = sec->base->include = 0;
+			free(dst);
+			continue;
+		}
+
+		/* overwrite with new relas list */
+		list_replace(&newrelas, &sec->relas);
+
+		/*
+		 * Update text section data buf and size.
+		 *
+		 * The rela section's data buf and size will be regenerated in
+		 * kpatch_rebuild_rela_section_data().
+		 */
+		sec->base->data->d_buf = dst;
+		sec->base->data->d_size = dst_offset;
+	}
+}
+
 static int is_null_sym(struct symbol *sym)
 {
 	return !strlen(sym->name);
@@ -2083,6 +2192,8 @@ int main(int argc, char *argv[])
 
 	log_debug("Process special sections\n");
 	kpatch_process_special_sections(kelf_patched);
+	log_debug("Strip undefined elements of known size\n");
+	livepatch_strip_undefined_elements(kelf_patched);
 	log_debug("Verify patchability\n");
 	kpatch_verify_patchability(kelf_patched);
 
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



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

Re: [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[Konrad Rzeszutek Wilk]

On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote:
> When there is no changed function in the generated payload, do not
> create an empty .livepatch.funcs section. Hypervisor code considers
> such payloads as broken and rejects to load them.
> 
> Such payloads without any changed functions may appear when only
> hooks are specified.

Ross, I am going to push this in next week unless you have other thoughts?

> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> 
> CR: https://code.amazon.com/reviews/CR-7368634
> ---
>  create-diff-object.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 82f777e..af2245c 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -1744,6 +1744,11 @@ static void livepatch_create_patches_sections(struct kpatch_elf *kelf,
>  		if (sym->type == STT_FUNC && sym->status == CHANGED)
>  			nr++;
>  
> +	if (nr == 0) {
> +		log_debug("No changed functions found. Skipping .livepatch.funcs section creation\n");
> +		return;
> +	}
> +
>  	/* create text/rela section pair */
>  	sec = create_section_pair(kelf, ".livepatch.funcs", sizeof(*funcs), nr);
>  	relasec = sec->rela;
> -- 
> 2.16.5
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
> Ust-ID: DE 289 237 879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
> 
> 

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

Re: [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[Andrew Cooper]

On 25/04/2019 05:51, Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote:
>> When there is no changed function in the generated payload, do not
>> create an empty .livepatch.funcs section. Hypervisor code considers
>> such payloads as broken and rejects to load them.
>>
>> Such payloads without any changed functions may appear when only
>> hooks are specified.
> Ross, I am going to push this in next week unless you have other thoughts?

No objections, but as the CR: links are internal links, which get the
following message when followed:

> Non-Amazon Employees, you have been mistakenly directed to an
> internal-only Amazon system. If you were given a link that brought you
> to this page, please let the source responsible for the link know that
> the link they have provided does not work for non-Amazon employees.

They should be stripped out of the eventual commit messages.

~Andrew

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

Re: [Xen-devel] [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[Andrew Cooper]

On 25/04/2019 05:51, Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote:
>> When there is no changed function in the generated payload, do not
>> create an empty .livepatch.funcs section. Hypervisor code considers
>> such payloads as broken and rejects to load them.
>>
>> Such payloads without any changed functions may appear when only
>> hooks are specified.
> Ross, I am going to push this in next week unless you have other thoughts?

No objections, but as the CR: links are internal links, which get the
following message when followed:

> Non-Amazon Employees, you have been mistakenly directed to an
> internal-only Amazon system. If you were given a link that brought you
> to this page, please let the source responsible for the link know that
> the link they have provided does not work for non-Amazon employees.

They should be stripped out of the eventual commit messages.

~Andrew

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

Re: [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[Ross Lagerwall]

On 4/25/19 5:51 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote:
>> When there is no changed function in the generated payload, do not
>> create an empty .livepatch.funcs section. Hypervisor code considers
>> such payloads as broken and rejects to load them.
>>
>> Such payloads without any changed functions may appear when only
>> hooks are specified.
> 
> Ross, I am going to push this in next week unless you have other thoughts?
> 
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

This code change looks OK to me. However:

1) I think that the hypervisor should treat an empty .livepatch.funcs 
section the same as it treats a non-present .livepatch.funcs section 
(i.e. it allows it) which would make this change unnecessary.

2) Unless I'm being stupid, I don't see how this change would work 
anyway. Surely this code at the start of prepare_payload() would fail if 
the section were missing?

     sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
     ASSERT(sec);
     if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
         return -EINVAL;

Regards,
-- 
Ross Lagerwall

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

Re: [livepatch-build-tools part3 1/3] create-diff-object: Do not create empty .livepatch.funcs section

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 16:37, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/25/19 5:51 AM, Konrad Rzeszutek Wilk wrote:
>> On Tue, Apr 16, 2019 at 12:22:39PM +0000, Pawel Wieczorkiewicz wrote:
>>> When there is no changed function in the generated payload, do not
>>> create an empty .livepatch.funcs section. Hypervisor code considers
>>> such payloads as broken and rejects to load them.
>>> 
>>> Such payloads without any changed functions may appear when only
>>> hooks are specified.
>> Ross, I am going to push this in next week unless you have other thoughts?
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> This code change looks OK to me. However:
> 

Thank you for reviewing.

> 1) I think that the hypervisor should treat an empty .livepatch.funcs section the same as it treats a non-present .livepatch.funcs section (i.e. it allows it) which would make this change unnecessary.
> 

I do not have a strong opinion here, but it felt unnecessary (and a bit confusing) to generate an empty section.
Also I did not want to touch hypervisor code for this.

> 2) Unless I'm being stupid, I don't see how this change would work anyway. Surely this code at the start of prepare_payload() would fail if the section were missing?
> 
>    sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
>    ASSERT(sec);
>    if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
>        return -EINVAL;
> 

In my soon-to-be-upstreamed backlog I have the following commit:
livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence
Where I actually make use of this new functionality.

The main idea behind this change, was to enable generation of hooks-only hotpatch modules
(i.e. modules that does not patch anything, just trigger actions).

> Regards,
> -- 
> Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



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