create-diff-object: handle missing padding at end of special section

Commit Message

Roger Pau Monné April 14, 2023, 3:19 p.m. UTC

From: Josh Poimboeuf <[e-mail]>

The paravirt_patch_site struct has 12 bytes of data and 4 bytes of
padding, for a total of 16 bytes.  However, when laying out the structs
in the .parainstructions section, the vmlinux script only aligns before
each struct's data, not after.  So the last entry doesn't have the
4-byte padding, which breaks kpatch_regenerate_special_section()'s
assumption of a 16-byte struct, resulting in a memcpy past the end of
the section.

Fixes #747.

Signed-off-by: Josh Poimboeuf <[e-mail]>

This is commit:

c2dc3836e862 create-diff-object: handle missing padding at end of special section

In kpatch repository.

I've seen the .fixup section get an alignment of 16 but a size of 81,
which makes the error removed in this patch trigger.  Overall I'm not
sure why the original alignment check was done against the size of the
section, the alignment applies to the address of the section, not its
size.

Signed-off-by: Roger Pau Monné <[e-mail]>
---
Cc: Konrad Rzeszutek Wilk <[e-mail]>
Cc: Ross Lagerwall <[e-mail]>
---
 create-diff-object.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

Comments

Andrew Cooper April 14, 2023, 4:17 p.m. UTC | #1

On 14/04/2023 4:19 pm, Roger Pau Monne wrote:
> From: Josh Poimboeuf <[e-mail]>
>
> The paravirt_patch_site struct has 12 bytes of data and 4 bytes of
> padding, for a total of 16 bytes.  However, when laying out the structs
> in the .parainstructions section, the vmlinux script only aligns before
> each struct's data, not after.  So the last entry doesn't have the
> 4-byte padding, which breaks kpatch_regenerate_special_section()'s
> assumption of a 16-byte struct, resulting in a memcpy past the end of
> the section.
>
> Fixes #747.
>
> Signed-off-by: Josh Poimboeuf <[e-mail]>
>
> This is commit:
>
> c2dc3836e862 create-diff-object: handle missing padding at end of special section
>
> In kpatch repository.
>
> I've seen the .fixup section get an alignment of 16 but a size of 81,
> which makes the error removed in this patch trigger.  Overall I'm not
> sure why the original alignment check was done against the size of the
> section, the alignment applies to the address of the section, not its
> size.
>
> Signed-off-by: Roger Pau Monné <[e-mail]>

Seems like a clean backport, so FWIW

Acked-by: Andrew Cooper <[e-mail]>

However, surely we want a correction to Xen's linker file too, to stop
putting out a badly aligned section?

~Andrew

Roger Pau Monné April 17, 2023, 7:31 a.m. UTC | #2

On Fri, Apr 14, 2023 at 05:17:42PM +0100, Andrew Cooper wrote:
> On 14/04/2023 4:19 pm, Roger Pau Monne wrote:
> > From: Josh Poimboeuf <[e-mail]>
> >
> > The paravirt_patch_site struct has 12 bytes of data and 4 bytes of
> > padding, for a total of 16 bytes.  However, when laying out the structs
> > in the .parainstructions section, the vmlinux script only aligns before
> > each struct's data, not after.  So the last entry doesn't have the
> > 4-byte padding, which breaks kpatch_regenerate_special_section()'s
> > assumption of a 16-byte struct, resulting in a memcpy past the end of
> > the section.
> >
> > Fixes #747.
> >
> > Signed-off-by: Josh Poimboeuf <[e-mail]>
> >
> > This is commit:
> >
> > c2dc3836e862 create-diff-object: handle missing padding at end of special section
> >
> > In kpatch repository.
> >
> > I've seen the .fixup section get an alignment of 16 but a size of 81,
> > which makes the error removed in this patch trigger.  Overall I'm not
> > sure why the original alignment check was done against the size of the
> > section, the alignment applies to the address of the section, not its
> > size.
> >
> > Signed-off-by: Roger Pau Monné <[e-mail]>
> 
> Seems like a clean backport, so FWIW
> 
> Acked-by: Andrew Cooper <[e-mail]>
> 
> However, surely we want a correction to Xen's linker file too, to stop
> putting out a badly aligned section?

AFAICT that alignment comes from the per-function-section object files,
so that's before the linker has assembled the xen image.  And the
address of the section is indeed alignment to the value, so it's all
correct.

Even then, it's my understanding the alignment in sh_addralign applies
to the address of the section, not the size, so I'm confused as to why
create-diff-object was expecting section sizes to the aligned.  IMO
it would make sense to pad the start address so it's aligned to the
section requirements, but not the section size.

Regardless, it's indeed a clean backport from the change upstream so
we should take it.

Thanks, Roger.

Ross Lagerwall April 18, 2023, 1:51 p.m. UTC | #3

> From: Roger Pau Monne <[e-mail]>
> Sent: Friday, April 14, 2023 4:19 PM
> To: [e-mail] <[e-mail]>
> Cc: Josh Poimboeuf <[e-mail]>; Roger Pau Monne <[e-mail]>; Konrad Rzeszutek Wilk <[e-mail]>; Ross Lagerwall <[e-mail]>
> Subject: [PATCH] create-diff-object: handle missing padding at end of special section 
>  
> From: Josh Poimboeuf <[e-mail]>
> 
> The paravirt_patch_site struct has 12 bytes of data and 4 bytes of
> padding, for a total of 16 bytes.  However, when laying out the structs
> in the .parainstructions section, the vmlinux script only aligns before
> each struct's data, not after.  So the last entry doesn't have the
> 4-byte padding, which breaks kpatch_regenerate_special_section()'s
> assumption of a 16-byte struct, resulting in a memcpy past the end of
> the section.
> 
> Fixes #747.
> 
> Signed-off-by: Josh Poimboeuf <[e-mail]>
> 
> This is commit:
> 
> c2dc3836e862 create-diff-object: handle missing padding at end of special section
> 
> In kpatch repository.
> 
> I've seen the .fixup section get an alignment of 16 but a size of 81,
> which makes the error removed in this patch trigger.  Overall I'm not
> sure why the original alignment check was done against the size of the
> section, the alignment applies to the address of the section, not its
> size.
> 
> Signed-off-by: Roger Pau Monné <[e-mail]>
> ---

Reviewed-by: Ross Lagerwall <[e-mail]>

Patch

diff --git a/create-diff-object.c b/create-diff-object.c
index d8a003216096..67784642bcd7 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -1204,7 +1204,7 @@  static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 {
 	struct rela *rela, *safe;
 	char *src, *dest;
-	int group_size, src_offset, dest_offset, include, align, aligned_size;
+	int group_size, src_offset, dest_offset, include;
 
 	LIST_HEAD(newrelas);
 
@@ -1234,6 +1234,18 @@  static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 	for ( ; src_offset < sec->base->sh.sh_size; src_offset += group_size) {
 
 		group_size = special->group_size(kelf, src_offset);
+
+		/*
+		 * In some cases the struct has padding at the end to ensure
+		 * that all structs after it are properly aligned.  But the
+		 * last struct in the section may not be padded.  In that case,
+		 * shrink the group_size such that it still (hopefully)
+		 * contains the data but doesn't go past the end of the
+		 * section.
+		 */
+		if (src_offset + group_size > sec->base->sh.sh_size)
+			group_size = sec->base->sh.sh_size - src_offset;
+
 		include = should_keep_rela_group(sec, src_offset, group_size);
 
 		if (!include)
@@ -1269,12 +1281,6 @@  static void kpatch_regenerate_special_section(struct kpatch_elf *kelf,
 		dest_offset += group_size;
 	}
 
-	/* verify that group_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 (!dest_offset) {
 		/* no changed or global functions referenced */
 		sec->status = sec->base->status = SAME;