bootconfig: Fix unaligned access when building footer

[PATCH] bootconfig: Fix unaligned access when building footer

Posted by Ben Hutchings 2 months, 2 weeks ago

Currently we add padding between the bootconfig text and footer to
ensure that the footer is aligned within the initramfs image.
However, because only the bootconfig data is held in memory, not the
full initramfs image, the footer may not be naturally aligned in
memory.

This can result in an alignment fault (SIGBUS) when writing the footer
on some architectures, such as sparc.

Build the footer in a struct on the stack before adding it to the
buffer.

References: https://buildd.debian.org/status/fetch.php?pkg=linux&arch=sparc64&ver=6.16%7Erc7-1%7Eexp1&stamp=1753209801&raw=0
Signed-off-by: Ben Hutchings <benh@debian.org>
---
 tools/bootconfig/main.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 4988e23a1422..57c669d2aa90 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -11,6 +11,7 @@
 #include <string.h>
 #include <errno.h>
 #include <endian.h>
+#include <assert.h>
 
 #include <linux/bootconfig.h>
 
@@ -363,7 +364,12 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
-	char *buf, *data, *p;
+	struct {
+		uint32_t size;
+		uint32_t csum;
+		char magic[BOOTCONFIG_MAGIC_LEN];
+	} footer;
+	char *buf, *data;
 	size_t total_size;
 	struct stat stat;
 	const char *msg;
@@ -433,17 +439,13 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	size += pad;
 
 	/* Add a footer */
-	p = data + size;
-	*(uint32_t *)p = htole32(size);
-	p += sizeof(uint32_t);
+	footer.size = htole32(size);
+	footer.csum = htole32(csum);
+	memcpy(footer.magic, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
+	static_assert(sizeof(footer) == BOOTCONFIG_FOOTER_SIZE);
+	memcpy(data + size, &footer, BOOTCONFIG_FOOTER_SIZE);
 
-	*(uint32_t *)p = htole32(csum);
-	p += sizeof(uint32_t);
-
-	memcpy(p, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	p += BOOTCONFIG_MAGIC_LEN;
-
-	total_size = p - data;
+	total_size = size + BOOTCONFIG_FOOTER_SIZE;
 
 	ret = write(fd, data, total_size);
 	if (ret < total_size) {

Re: [PATCH] bootconfig: Fix unaligned access when building footer

Posted by Masami Hiramatsu (Google) 2 months, 2 weeks ago

On Wed, 23 Jul 2025 12:49:25 +0200
Ben Hutchings <benh@debian.org> wrote:

> Currently we add padding between the bootconfig text and footer to
> ensure that the footer is aligned within the initramfs image.
> However, because only the bootconfig data is held in memory, not the
> full initramfs image, the footer may not be naturally aligned in
> memory.
> 
> This can result in an alignment fault (SIGBUS) when writing the footer
> on some architectures, such as sparc.

Aah, got it. 

> 
> Build the footer in a struct on the stack before adding it to the
> buffer.
> 
> References: https://buildd.debian.org/status/fetch.php?pkg=linux&arch=sparc64&ver=6.16%7Erc7-1%7Eexp1&stamp=1753209801&raw=0
> Signed-off-by: Ben Hutchings <benh@debian.org>
> ---
>  tools/bootconfig/main.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 4988e23a1422..57c669d2aa90 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
> @@ -11,6 +11,7 @@
>  #include <string.h>
>  #include <errno.h>
>  #include <endian.h>
> +#include <assert.h>
>  
>  #include <linux/bootconfig.h>
>  
> @@ -363,7 +364,12 @@ static int delete_xbc(const char *path)
>  
>  static int apply_xbc(const char *path, const char *xbc_path)
>  {
> -	char *buf, *data, *p;
> +	struct {
> +		uint32_t size;
> +		uint32_t csum;
> +		char magic[BOOTCONFIG_MAGIC_LEN];
> +	} footer;

Don't we need __attribute__((__packed__)) for the footer?

Thank you,

> +	char *buf, *data;
>  	size_t total_size;
>  	struct stat stat;
>  	const char *msg;
> @@ -433,17 +439,13 @@ static int apply_xbc(const char *path, const char *xbc_path)
>  	size += pad;
>  
>  	/* Add a footer */
> -	p = data + size;
> -	*(uint32_t *)p = htole32(size);
> -	p += sizeof(uint32_t);
> +	footer.size = htole32(size);
> +	footer.csum = htole32(csum);
> +	memcpy(footer.magic, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> +	static_assert(sizeof(footer) == BOOTCONFIG_FOOTER_SIZE);
> +	memcpy(data + size, &footer, BOOTCONFIG_FOOTER_SIZE);
>  
> -	*(uint32_t *)p = htole32(csum);
> -	p += sizeof(uint32_t);
> -
> -	memcpy(p, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> -	p += BOOTCONFIG_MAGIC_LEN;
> -
> -	total_size = p - data;
> +	total_size = size + BOOTCONFIG_FOOTER_SIZE;
>  
>  	ret = write(fd, data, total_size);
>  	if (ret < total_size) {


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH] bootconfig: Fix unaligned access when building footer

Posted by Ben Hutchings 2 months, 2 weeks ago

On Thu, 2025-07-24 at 00:37 +0900, Masami Hiramatsu wrote:
> On Wed, 23 Jul 2025 12:49:25 +0200
> Ben Hutchings <benh@debian.org> wrote:
[...]
> >  static int apply_xbc(const char *path, const char *xbc_path)
> >  {
> > -	char *buf, *data, *p;
> > +	struct {
> > +		uint32_t size;
> > +		uint32_t csum;
> > +		char magic[BOOTCONFIG_MAGIC_LEN];
> > +	} footer;
> 
> Don't we need __attribute__((__packed__)) for the footer?

I don't see any reason for there to be padding in this structure, since
it has an alignment of 4 and the size of each member is a multiple of 4.

I included an assertion that there is no padding:

[...]
> > +	memcpy(footer.magic, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> > +	static_assert(sizeof(footer) == BOOTCONFIG_FOOTER_SIZE);
> > +	memcpy(data + size, &footer, BOOTCONFIG_FOOTER_SIZE);
[...]

Ben.

-- 
Ben Hutchings - Debian developer, member of kernel, installer and LTS
teams

Re: [PATCH] bootconfig: Fix unaligned access when building footer

Posted by Masami Hiramatsu (Google) 2 months, 2 weeks ago

On Wed, 23 Jul 2025 22:20:35 +0200
Ben Hutchings <benh@debian.org> wrote:

> On Thu, 2025-07-24 at 00:37 +0900, Masami Hiramatsu wrote:
> > On Wed, 23 Jul 2025 12:49:25 +0200
> > Ben Hutchings <benh@debian.org> wrote:
> [...]
> > >  static int apply_xbc(const char *path, const char *xbc_path)
> > >  {
> > > -	char *buf, *data, *p;
> > > +	struct {
> > > +		uint32_t size;
> > > +		uint32_t csum;
> > > +		char magic[BOOTCONFIG_MAGIC_LEN];
> > > +	} footer;
> > 
> > Don't we need __attribute__((__packed__)) for the footer?
> 
> I don't see any reason for there to be padding in this structure, since
> it has an alignment of 4 and the size of each member is a multiple of 4.
> 
> I included an assertion that there is no padding:
> 
> [...]
> > > +	memcpy(footer.magic, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
> > > +	static_assert(sizeof(footer) == BOOTCONFIG_FOOTER_SIZE);
> > > +	memcpy(data + size, &footer, BOOTCONFIG_FOOTER_SIZE);
> [...]

OK, if we saw the assertion error, we'll add packed attribute.
Let me pick it as it is.

Thank you!


> 
> Ben.
> 
> -- 
> Ben Hutchings - Debian developer, member of kernel, installer and LTS
> teams


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>