[PATCH v3 0/3] tools/bootconfig: Align the bootconfig applied initrd

[PATCH v3 0/3] tools/bootconfig: Align the bootconfig applied initrd

[Masami Hiramatsu]

Hi,

This is the 3rd version of the bootconfig tool update to align
the total size of initrd + bootconfig to 4.

Previous version is here;

 https://lkml.kernel.org/r/160554375807.96595.16142008590130221699.stgit@devnote2

This version adds a new partial-write fix patch as [1/3], and
update main patch [2/3] by unifying writeU) and check write error
correctly as Linus pointed.

To adjust the file size, the bootconfig tool adds padding null
characters in between the boot configuration data and the footer.

The changing points are
- The bootconfig applied initrd image size is aligned to 4.
- To insert the padding null ('\0') bytes, the size in the footer
  can be bigger than the actual bootconfig file size.
- But the max size of the boot configuration file is same, because
  the max size doesn't include the last null characters.

In this series I keep 4 bytes aligned instead of longer size,
because only I could found was that the grub might align the initrd
filesize to 4, and U-Boot/EDK2 would not change it. So I couldn't
say what is the best size.

Anyway, I updated the documentation too, which clearly says that
the above changing points, and if the bootloader pass the wrong
size, kernel will not find bootconfig from the initrd.

Thank you,

---

Masami Hiramatsu (3):
      tools/bootconfig: Fix to check the write failure correctly
      tools/bootconfig: Align the bootconfig applied initrd image size to 4
      docs: bootconfig: Update file format on initrd image


 Documentation/admin-guide/bootconfig.rst |   18 +++++++--
 include/linux/bootconfig.h               |    1 +
 tools/bootconfig/main.c                  |   57 ++++++++++++++++++++++--------
 tools/bootconfig/test-bootconfig.sh      |    6 +++
 4 files changed, 60 insertions(+), 22 deletions(-)

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

[PATCH v3 1/3] tools/bootconfig: Fix to check the write failure correctly

[Masami Hiramatsu]

Fix to check the write(2) failure including partial write
correctly and try to rollback the partial write, because
if there is no BOOTCONFIG_MAGIC string, we can not remove it.

Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 tools/bootconfig/main.c |   27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index eb92027817a7..f7b89a50404c 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -332,6 +332,7 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
+	struct stat stat;
 	u32 size, csum;
 	char *buf, *data;
 	int ret, fd;
@@ -388,16 +389,26 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		return fd;
 	}
 	/* TODO: Ensure the @path is initramfs/initrd image */
+	if (fstat(fd, &stat) < 0) {
+		pr_err("Failed to get the size of %s\n", path);
+		goto out;
+	}
 	ret = write(fd, data, size + 8);
-	if (ret < 0) {
+	if (ret < size + 8) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		goto out;
+		if (ret < 0)
+			goto out;
+		goto out_rollback;
 	}
 	/* Write a magic word of the bootconfig */
 	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < 0) {
+	if (ret < BOOTCONFIG_MAGIC_LEN) {
+		if (ret < 0)
+			ret = -errno;
 		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out;
+		goto out_rollback;
 	}
 	ret = 0;
 out:
@@ -405,6 +416,14 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	free(data);
 
 	return ret;
+
+out_rollback:
+	if (ftruncate(fd, stat.st_size) < 0) {
+		ret = -errno;
+		pr_err("Failed to rollback the write error: %d\n", ret);
+		pr_err("The initrd %s may be corrupted. Recommend to rebuild.\n", path);
+	}
+	goto out;
 }
 
 static int usage(void)

Re: [PATCH v3 1/3] tools/bootconfig: Fix to check the write failure correctly

[Chen Yu]

On Tue, Nov 17, 2020 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Fix to check the write(2) failure including partial write
> correctly and try to rollback the partial write, because
> if there is no BOOTCONFIG_MAGIC string, we can not remove it.
>
> Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
For [1/3] and [2/3]
Tested-by: Chen Yu <yu.chen.surf@gmail.com>

-- 
Thanks,
Chenyu

Re: [PATCH v3 1/3] tools/bootconfig: Fix to check the write failure correctly

[Masami Hiramatsu]

On Tue, 17 Nov 2020 20:10:10 +0800
Chen Yu <yu.chen.surf@gmail.com> wrote:

> On Tue, Nov 17, 2020 at 5:53 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Fix to check the write(2) failure including partial write
> > correctly and try to rollback the partial write, because
> > if there is no BOOTCONFIG_MAGIC string, we can not remove it.
> >
> > Fixes: 85c46b78da58 ("bootconfig: Add bootconfig magic word for indicating bootconfig explicitly")
> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> For [1/3] and [2/3]
> Tested-by: Chen Yu <yu.chen.surf@gmail.com>

Thank you Chen,

BTW, I found another bug in bootconfig error handling.
Steve, did you already pick this series? If not yet, I will update the seires to add that fix.

Thank you,

> 
> -- 
> Thanks,
> Chenyu


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 1/3] tools/bootconfig: Fix to check the write failure correctly

[Steven Rostedt]

On Wed, 18 Nov 2020 22:37:08 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> BTW, I found another bug in bootconfig error handling.
> Steve, did you already pick this series? If not yet, I will update the seires to add that fix.

No, I was about to pull it in today. I'll wait for your new series.

Thanks!

-- Steve

[PATCH v3 2/3] tools/bootconfig: Align the bootconfig applied initrd image size to 4

[Masami Hiramatsu]

Align the bootconfig applied initrd image size to 4. To fill the gap,
the bootconfig command uses null characters in between the bootconfig
data and the footer. This will expands the footer size but don't change
the checksum.
Thus the block image of the initrd file with bootconfig is as follows.

[initrd][bootconfig][(pad)][size][csum]["#BOOTCONFIG\n"]

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v3:
  - Fix patch description
  - Fix a typo.
  - Consolidate several write()s to 1 time write to fix/simplify
    the error check.
 Changes in v2:
  - Fix to add the footer size.
---
 include/linux/bootconfig.h          |    1 +
 tools/bootconfig/main.c             |   48 ++++++++++++++++++++---------------
 tools/bootconfig/test-bootconfig.sh |    6 ++++
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 9903088891fa..461f621047f3 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -12,6 +12,7 @@
 
 #define BOOTCONFIG_MAGIC	"#BOOTCONFIG\n"
 #define BOOTCONFIG_MAGIC_LEN	12
+#define BOOTCONFIG_ALIGN	4
 
 /* XBC tree node */
 struct xbc_node {
diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index f7b89a50404c..3991754c97e9 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -332,12 +332,13 @@ static int delete_xbc(const char *path)
 
 static int apply_xbc(const char *path, const char *xbc_path)
 {
+	size_t total_size;
 	struct stat stat;
 	u32 size, csum;
 	char *buf, *data;
 	int ret, fd;
 	const char *msg;
-	int pos;
+	int pos, pad;
 
 	ret = load_xbc_file(xbc_path, &buf);
 	if (ret < 0) {
@@ -347,13 +348,12 @@ static int apply_xbc(const char *path, const char *xbc_path)
 	size = strlen(buf) + 1;
 	csum = checksum((unsigned char *)buf, size);
 
-	/* Prepare xbc_path data */
-	data = malloc(size + 8);
+	/* Backup the bootconfig data */
+	data = calloc(size + BOOTCONFIG_ALIGN +
+		      sizeof(u32) + sizeof(u32) + BOOTCONFIG_MAGIC_LEN, 1);
 	if (!data)
 		return -ENOMEM;
-	strcpy(data, buf);
-	*(u32 *)(data + size) = size;
-	*(u32 *)(data + size + 4) = csum;
+	memcpy(data, buf, size);
 
 	/* Check the data format */
 	ret = xbc_init(buf, &msg, &pos);
@@ -393,24 +393,30 @@ static int apply_xbc(const char *path, const char *xbc_path)
 		pr_err("Failed to get the size of %s\n", path);
 		goto out;
 	}
-	ret = write(fd, data, size + 8);
-	if (ret < size + 8) {
+
+	/* To align up the total size to BOOTCONFIG_ALIGN, get padding size */
+	total_size = stat.st_size + size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
+	pad = BOOTCONFIG_ALIGN - total_size % BOOTCONFIG_ALIGN;
+	if (pad == BOOTCONFIG_ALIGN)
+		pad = 0;
+	size += pad;
+
+	/* Add a footer */
+	*(u32 *)(data + size) = size;
+	*(u32 *)(data + size + sizeof(u32)) = csum;
+	memcpy(data + size + sizeof(u32) * 2, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
+	total_size = size + sizeof(u32) * 2 + BOOTCONFIG_MAGIC_LEN;
+
+	ret = write(fd, data, total_size);
+	if (ret < total_size) {
 		if (ret < 0)
 			ret = -errno;
 		pr_err("Failed to apply a boot config: %d\n", ret);
-		if (ret < 0)
-			goto out;
-		goto out_rollback;
-	}
-	/* Write a magic word of the bootconfig */
-	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);
-	if (ret < BOOTCONFIG_MAGIC_LEN) {
-		if (ret < 0)
-			ret = -errno;
-		pr_err("Failed to apply a boot config magic: %d\n", ret);
-		goto out_rollback;
-	}
-	ret = 0;
+		if (ret > 0)
+			goto out_rollback;
+	} else
+		ret = 0;
+
 out:
 	close(fd);
 	free(data);
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index d295e406a756..baed891d0ba4 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -9,6 +9,7 @@ else
   TESTDIR=.
 fi
 BOOTCONF=${TESTDIR}/bootconfig
+ALIGN=4
 
 INITRD=`mktemp ${TESTDIR}/initrd-XXXX`
 TEMPCONF=`mktemp ${TESTDIR}/temp-XXXX.bconf`
@@ -59,7 +60,10 @@ echo "Show command test"
 xpass $BOOTCONF $INITRD
 
 echo "File size check"
-xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)
+total_size=$(expr $bconf_size + $initrd_size + 9 + 12 + $ALIGN - 1 )
+total_size=$(expr $total_size / $ALIGN)
+total_size=$(expr $total_size \* $ALIGN)
+xpass test $new_size -eq $total_size
 
 echo "Apply command repeat test"
 xpass $BOOTCONF -a $TEMPCONF $INITRD

[PATCH v3 3/3] docs: bootconfig: Update file format on initrd image

[Masami Hiramatsu]

To align the total file size, add padding null character when appending
the bootconfig to initrd image.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index a22024f9175e..363599683784 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -137,15 +137,22 @@ Boot Kernel With a Boot Config
 ==============================
 
 Since the boot configuration file is loaded with initrd, it will be added
-to the end of the initrd (initramfs) image file with size, checksum and
-12-byte magic word as below.
+to the end of the initrd (initramfs) image file with padding, size,
+checksum and 12-byte magic word as below.
 
-[initrd][bootconfig][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+[initrd][bootconfig][padding][size(u32)][checksum(u32)][#BOOTCONFIG\n]
+
+When the boot configuration is added to the initrd image, the total
+file size is aligned to 4 bytes. To fill the gap, null characters
+(``\0``) will be added. Thus the ``size`` is the length of the bootconfig
+file + padding bytes.
 
 The Linux kernel decodes the last part of the initrd image in memory to
 get the boot configuration data.
 Because of this "piggyback" method, there is no need to change or
-update the boot loader and the kernel image itself.
+update the boot loader and the kernel image itself as long as the boot
+loader passes the correct initrd file size. If by any chance, the boot
+loader passes a longer size, the kernel feils to find the bootconfig data.
 
 To do this operation, Linux kernel provides "bootconfig" command under
 tools/bootconfig, which allows admin to apply or delete the config file
@@ -176,7 +183,8 @@ up to 512 key-value pairs. If keys contains 3 words in average, it can
 contain 256 key-value pairs. In most cases, the number of config items
 will be under 100 entries and smaller than 8KB, so it would be enough.
 If the node number exceeds 1024, parser returns an error even if the file
-size is smaller than 32KB.
+size is smaller than 32KB. (Note that this maximum size is not including
+the padding null characters.)
 Anyway, since bootconfig command verifies it when appending a boot config
 to initrd image, user can notice it before boot.