diff mbox series

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

Message ID 20190808125132.10484-1-wipawel@amazon.de (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Wieczorkiewicz, Pawel Aug. 8, 2019, 12:51 p.m. UTC
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 undefined (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>
---
v2:
* Kept lines limits to 80 chars (whereever possible)
* Fixed commit message
---
 create-diff-object.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 129 insertions(+), 2 deletions(-)

Comments

Ross Lagerwall Aug. 16, 2019, 3:02 p.m. UTC | #1
On 8/8/19 1:51 PM, Pawel Wieczorkiewicz wrote:
> 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 undefined (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.

The code in this patch seems to be very similar (i.e. somewhat 
copy-and-pasted) to kpatch_regenerate_special_section() which prunes the 
rela list and rebuilds the corresponding text section according to the 
predicate should_keep_rela_group(). The intent of the function also 
seems to be the same (only keep elements that are needed).

In what situations does the existing function not do the right thing?

Can should_keep_rela_group() be updated instead?

Thanks,
Wieczorkiewicz, Pawel Aug. 19, 2019, 10:55 a.m. UTC | #2
On 16. Aug 2019, at 17:02, Ross Lagerwall <ross.lagerwall@citrix.com<mailto:ross.lagerwall@citrix.com>> wrote:

On 8/8/19 1:51 PM, Pawel Wieczorkiewicz wrote:
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 undefined (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.

The code in this patch seems to be very similar (i.e. somewhat copy-and-pasted) to kpatch_regenerate_special_section() which prunes the rela list and rebuilds the corresponding text section according to the predicate should_keep_rela_group(). The intent of the function also seems to be the same (only keep elements that are needed).

In what situations does the existing function not do the right thing?

Can should_keep_rela_group() be updated instead?


Yes, the code is largely based on the kpatch_regenerate_special_section(). However, the livepatch_strip_undefined_elements() and kpatch_regenerate_special_section() have different "granularity" and different scope.
1. Scope
- livepatch_strip_undefined_elements(): all RELA sections
- kpatch_regenerate_special_section(): special RELA sections

2. "granularity"
- livepatch_strip_undefined_elements(): all relas to be checked
- kpatch_regenerate_special_section(): all relas in a group are copied

I think it should be possible to create a common function with the base functionality that could take most of the common code in and then be used from within both of the functions.
However, I did not want to touch existing functionality and I am afraid the changes may result in a pretty convoluted code.

If you think it is still worth it, I can give it a shot.

Thanks,
--
Ross Lagerwall


Best Regards,
Pawel Wieczorkiewicz



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

Patch

diff --git a/create-diff-object.c b/create-diff-object.c
index e905131..f352704 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1555,6 +1555,13 @@  static inline int get_section_entry_size(const struct section *sec,
 	return entry_size;
 }
 
+/* Check if RELA entry has undefined or unchanged/not-included symbols. */
+static inline bool has_rela_undefined_symbol(const struct rela *rela)
+{
+	return (GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
+	       (!rela->sym->include && (rela->sym->status == SAME));
+}
+
 static int kpatch_section_has_undef_symbols(struct kpatch_elf *kelf,
 					    const struct section *sec)
 {
@@ -1574,8 +1581,7 @@  static int kpatch_section_has_undef_symbols(struct kpatch_elf *kelf,
 				continue;
 			}
 
-			if ((GELF_R_SYM(rela->rela.r_info) == STN_UNDEF) ||
-			    (!rela->sym->include && rela->sym->status == SAME)) {
+			if (has_rela_undefined_symbol(rela)) {
 				log_normal("section %s has an entry with a STN_UNDEF symbol: %s\n",
 					   sec->name, rela->sym->name ?: "none");
 				return true;
@@ -1989,6 +1995,125 @@  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_symbol(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);
@@ -2185,6 +2310,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);