This patchset adds basic kunit test coverage for initramfs unpacking and cleans up some minor buffer handling issues / inefficiencies.
Changes since v2 (patch 2 only): - fix !CONFIG_INITRAMFS_PRESERVE_MTIME kunit test checks - add test MODULE_DESCRIPTION(), as suggested by Jeff Johnson - add some missing headers, reported by kernel test robot
Changes since v1 (RFC): - rebase atop v6.12-rc6 and filename field overrun fix from https://lore.kernel.org/r/20241030035509.20194-2-ddiss@suse.de - add unit test coverage (new patches 1 and 2) - add patch: fix hardlink hash leak without TRAILER - rework patch: avoid static buffer for error message + drop unnecessary message propagation - drop patch: cpio_buf reuse for built-in and bootloader initramfs + no good justification for the change
Feedback appreciated.
David Disseldorp (9): init: add initramfs_internal.h initramfs_test: kunit tests for initramfs unpacking vsprintf: add simple_strntoul initramfs: avoid memcpy for hex header fields initramfs: remove extra symlink path buffer initramfs: merge header_buf and name_buf initramfs: reuse name_len for dir mtime tracking initramfs: fix hardlink hash leak without TRAILER initramfs: avoid static buffer for error message
include/linux/kstrtox.h | 1 + init/.kunitconfig | 3 + init/Kconfig | 7 + init/Makefile | 1 + init/initramfs.c | 73 +++++---- init/initramfs_internal.h | 8 + init/initramfs_test.c | 403 ++++++++++++++++++++++++++++++++++++++++++++++ lib/vsprintf.c | 7 + 8 files changed, 471 insertions(+), 32 deletions(-) create mode 100644 init/.kunitconfig create mode 100644 init/initramfs_internal.h create mode 100644 init/initramfs_test.c
The new header only exports a single unpack function and a CPIO_HDRLEN constant for future test use.
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 16 +++++++++++++--- init/initramfs_internal.h | 8 ++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 init/initramfs_internal.h
diff --git a/init/initramfs.c b/init/initramfs.c index b2f7583bb1f5c..002e83ae12ac7 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -20,6 +20,7 @@ #include <linux/security.h>
#include "do_mounts.h" +#include "initramfs_internal.h"
static __initdata bool csum_present; static __initdata u32 io_csum; @@ -256,7 +257,7 @@ static __initdata char *header_buf, *symlink_buf, *name_buf;
static int __init do_start(void) { - read_into(header_buf, 110, GotHeader); + read_into(header_buf, CPIO_HDRLEN, GotHeader); return 0; }
@@ -497,14 +498,23 @@ static unsigned long my_inptr __initdata; /* index of next byte to be processed
#include <linux/decompress/generic.h>
-static char * __init unpack_to_rootfs(char *buf, unsigned long len) +/** + * unpack_to_rootfs - decompress and extract an initramfs archive + * @buf: input initramfs archive to extract + * @len: length of initramfs data to process + * + * Returns: NULL for success or an error message string + * + * This symbol shouldn't be used externally. It's available for unit tests. + */ +char * __init unpack_to_rootfs(char *buf, unsigned long len) { long written; decompress_fn decompress; const char *compress_name; static __initdata char msg_buf[64];
- header_buf = kmalloc(110, GFP_KERNEL); + header_buf = kmalloc(CPIO_HDRLEN, GFP_KERNEL); symlink_buf = kmalloc(PATH_MAX + N_ALIGN(PATH_MAX) + 1, GFP_KERNEL); name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL);
diff --git a/init/initramfs_internal.h b/init/initramfs_internal.h new file mode 100644 index 0000000000000..233dad16b0a00 --- /dev/null +++ b/init/initramfs_internal.h @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: GPL-2.0 +#ifndef __INITRAMFS_INTERNAL_H__ +#define __INITRAMFS_INTERNAL_H__ + +char *unpack_to_rootfs(char *buf, unsigned long len); +#define CPIO_HDRLEN 110 + +#endif
Provide some basic initramfs unpack sanity tests covering: - simple file / dir extraction - filename field overrun, as reported and fixed separately via https://lore.kernel.org/r/20241030035509.20194-2-ddiss@suse.de - "070702" cpio data checksums - hardlinks
Signed-off-by: David Disseldorp ddiss@suse.de --- init/.kunitconfig | 3 + init/Kconfig | 7 + init/Makefile | 1 + init/initramfs_test.c | 408 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 419 insertions(+) create mode 100644 init/.kunitconfig create mode 100644 init/initramfs_test.c
diff --git a/init/.kunitconfig b/init/.kunitconfig new file mode 100644 index 0000000000000..acb906b1a5f98 --- /dev/null +++ b/init/.kunitconfig @@ -0,0 +1,3 @@ +CONFIG_KUNIT=y +CONFIG_BLK_DEV_INITRD=y +CONFIG_INITRAMFS_TEST=y diff --git a/init/Kconfig b/init/Kconfig index c521e1421ad4a..cf8327cdd6739 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -1431,6 +1431,13 @@ config INITRAMFS_PRESERVE_MTIME
If unsure, say Y.
+config INITRAMFS_TEST + bool "Test initramfs cpio archive extraction" if !KUNIT_ALL_TESTS + depends on BLK_DEV_INITRD && KUNIT=y + default KUNIT_ALL_TESTS + help + Build KUnit tests for initramfs. See Documentation/dev-tools/kunit + choice prompt "Compiler optimization level" default CC_OPTIMIZE_FOR_PERFORMANCE diff --git a/init/Makefile b/init/Makefile index 10b652d33e872..d6f75d8907e09 100644 --- a/init/Makefile +++ b/init/Makefile @@ -12,6 +12,7 @@ else obj-$(CONFIG_BLK_DEV_INITRD) += initramfs.o endif obj-$(CONFIG_GENERIC_CALIBRATE_DELAY) += calibrate.o +obj-$(CONFIG_INITRAMFS_TEST) += initramfs_test.o
obj-y += init_task.o
diff --git a/init/initramfs_test.c b/init/initramfs_test.c new file mode 100644 index 0000000000000..84b21f465bc3d --- /dev/null +++ b/init/initramfs_test.c @@ -0,0 +1,408 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <kunit/test.h> +#include <linux/fcntl.h> +#include <linux/file.h> +#include <linux/fs.h> +#include <linux/init_syscalls.h> +#include <linux/stringify.h> +#include <linux/timekeeping.h> +#include "initramfs_internal.h" + +struct initramfs_test_cpio { + char *magic; + unsigned int ino; + unsigned int mode; + unsigned int uid; + unsigned int gid; + unsigned int nlink; + unsigned int mtime; + unsigned int filesize; + unsigned int devmajor; + unsigned int devminor; + unsigned int rdevmajor; + unsigned int rdevminor; + unsigned int namesize; + unsigned int csum; + char *fname; + char *data; +}; + +static size_t fill_cpio(struct initramfs_test_cpio *cs, size_t csz, char *out) +{ + int i; + size_t off = 0; + + for (i = 0; i < csz; i++) { + char *pos = &out[off]; + struct initramfs_test_cpio *c = &cs[i]; + size_t thislen; + + /* +1 to account for nulterm */ + thislen = sprintf(pos, "%s" + "%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x" + "%s", + c->magic, c->ino, c->mode, c->uid, c->gid, c->nlink, + c->mtime, c->filesize, c->devmajor, c->devminor, + c->rdevmajor, c->rdevminor, c->namesize, c->csum, + c->fname) + 1; + pr_debug("packing (%zu): %.*s\n", thislen, (int)thislen, pos); + off += thislen; + while (off & 3) + out[off++] = '\0'; + + memcpy(&out[off], c->data, c->filesize); + off += c->filesize; + while (off & 3) + out[off++] = '\0'; + } + + return off; +} + +static void __init initramfs_test_extract(struct kunit *test) +{ + char *err, *cpio_srcbuf; + size_t len; + struct timespec64 ts_before, ts_after; + struct kstat st = {}; + struct initramfs_test_cpio c[] = { { + .magic = "070701", + .ino = 1, + .mode = S_IFREG | 0777, + .uid = 12, + .gid = 34, + .nlink = 1, + .mtime = 56, + .filesize = 0, + .devmajor = 0, + .devminor = 1, + .rdevmajor = 0, + .rdevminor = 0, + .namesize = sizeof("initramfs_test_extract"), + .csum = 0, + .fname = "initramfs_test_extract", + }, { + .magic = "070701", + .ino = 2, + .mode = S_IFDIR | 0777, + .nlink = 1, + .mtime = 57, + .devminor = 1, + .namesize = sizeof("initramfs_test_extract_dir"), + .fname = "initramfs_test_extract_dir", + }, { + .magic = "070701", + .namesize = sizeof("TRAILER!!!"), + .fname = "TRAILER!!!", + } }; + + /* +3 to cater for any 4-byte end-alignment */ + cpio_srcbuf = kzalloc(ARRAY_SIZE(c) * (CPIO_HDRLEN + PATH_MAX + 3), + GFP_KERNEL); + len = fill_cpio(c, ARRAY_SIZE(c), cpio_srcbuf); + + ktime_get_real_ts64(&ts_before); + err = unpack_to_rootfs(cpio_srcbuf, len); + ktime_get_real_ts64(&ts_after); + if (err) { + KUNIT_FAIL(test, "unpack failed %s", err); + goto out; + } + + KUNIT_EXPECT_EQ(test, init_stat(c[0].fname, &st, 0), 0); + KUNIT_EXPECT_TRUE(test, S_ISREG(st.mode)); + KUNIT_EXPECT_TRUE(test, uid_eq(st.uid, KUIDT_INIT(c[0].uid))); + KUNIT_EXPECT_TRUE(test, gid_eq(st.gid, KGIDT_INIT(c[0].gid))); + KUNIT_EXPECT_EQ(test, st.nlink, 1); + if (IS_ENABLED(CONFIG_INITRAMFS_PRESERVE_MTIME)) { + KUNIT_EXPECT_EQ(test, st.mtime.tv_sec, c[0].mtime); + } else { + KUNIT_EXPECT_GE(test, st.mtime.tv_sec, ts_before.tv_sec); + KUNIT_EXPECT_LE(test, st.mtime.tv_sec, ts_after.tv_sec); + } + KUNIT_EXPECT_EQ(test, st.blocks, c[0].filesize); + + KUNIT_EXPECT_EQ(test, init_stat(c[1].fname, &st, 0), 0); + KUNIT_EXPECT_TRUE(test, S_ISDIR(st.mode)); + if (IS_ENABLED(CONFIG_INITRAMFS_PRESERVE_MTIME)) { + KUNIT_EXPECT_EQ(test, st.mtime.tv_sec, c[1].mtime); + } else { + KUNIT_EXPECT_GE(test, st.mtime.tv_sec, ts_before.tv_sec); + KUNIT_EXPECT_LE(test, st.mtime.tv_sec, ts_after.tv_sec); + } + + KUNIT_EXPECT_EQ(test, init_unlink(c[0].fname), 0); + KUNIT_EXPECT_EQ(test, init_rmdir(c[1].fname), 0); +out: + kfree(cpio_srcbuf); +} + +/* + * Don't terminate filename. Previously, the cpio filename field was passed + * directly to filp_open(collected, O_CREAT|..) without nulterm checks. See + * https://lore.kernel.org/linux-fsdevel/20241030035509.20194-2-ddiss@suse.de + */ +static void __init initramfs_test_fname_overrun(struct kunit *test) +{ + char *err, *cpio_srcbuf; + size_t len, suffix_off; + struct initramfs_test_cpio c[] = { { + .magic = "070701", + .ino = 1, + .mode = S_IFREG | 0777, + .uid = 0, + .gid = 0, + .nlink = 1, + .mtime = 1, + .filesize = 0, + .devmajor = 0, + .devminor = 1, + .rdevmajor = 0, + .rdevminor = 0, + .namesize = sizeof("initramfs_test_fname_overrun"), + .csum = 0, + .fname = "initramfs_test_fname_overrun", + } }; + + /* + * poison cpio source buffer, so we can detect overrun. source + * buffer is used by read_into() when hdr or fname + * are already available (e.g. no compression). + */ + cpio_srcbuf = kmalloc(CPIO_HDRLEN + PATH_MAX + 3, GFP_KERNEL); + memset(cpio_srcbuf, 'B', CPIO_HDRLEN + PATH_MAX + 3); + /* limit overrun to avoid crashes / filp_open() ENAMETOOLONG */ + cpio_srcbuf[CPIO_HDRLEN + strlen(c[0].fname) + 20] = '\0'; + + len = fill_cpio(c, ARRAY_SIZE(c), cpio_srcbuf); + /* overwrite trailing fname terminator and padding */ + suffix_off = len - 1; + while (cpio_srcbuf[suffix_off] == '\0') { + cpio_srcbuf[suffix_off] = 'P'; + suffix_off--; + } + + err = unpack_to_rootfs(cpio_srcbuf, len); + KUNIT_EXPECT_NOT_NULL(test, err); + + kfree(cpio_srcbuf); +} + +static void __init initramfs_test_data(struct kunit *test) +{ + char *err, *cpio_srcbuf; + size_t len; + struct file *file; + struct initramfs_test_cpio c[] = { { + .magic = "070701", + .ino = 1, + .mode = S_IFREG | 0777, + .uid = 0, + .gid = 0, + .nlink = 1, + .mtime = 1, + .filesize = sizeof("ASDF") - 1, + .devmajor = 0, + .devminor = 1, + .rdevmajor = 0, + .rdevminor = 0, + .namesize = sizeof("initramfs_test_data"), + .csum = 0, + .fname = "initramfs_test_data", + .data = "ASDF", + } }; + + /* +6 for max name and data 4-byte padding */ + cpio_srcbuf = kmalloc(CPIO_HDRLEN + c[0].namesize + c[0].filesize + 6, + GFP_KERNEL); + + len = fill_cpio(c, ARRAY_SIZE(c), cpio_srcbuf); + + err = unpack_to_rootfs(cpio_srcbuf, len); + KUNIT_EXPECT_NULL(test, err); + + file = filp_open(c[0].fname, O_RDONLY, 0); + if (!file) { + KUNIT_FAIL(test, "open failed"); + goto out; + } + + /* read back file contents into @cpio_srcbuf and confirm match */ + len = kernel_read(file, cpio_srcbuf, c[0].filesize, NULL); + KUNIT_EXPECT_EQ(test, len, c[0].filesize); + KUNIT_EXPECT_MEMEQ(test, cpio_srcbuf, c[0].data, len); + + fput(file); + KUNIT_EXPECT_EQ(test, init_unlink(c[0].fname), 0); +out: + kfree(cpio_srcbuf); +} + +static void __init initramfs_test_csum(struct kunit *test) +{ + char *err, *cpio_srcbuf; + size_t len; + struct initramfs_test_cpio c[] = { { + /* 070702 magic indicates a valid csum is present */ + .magic = "070702", + .ino = 1, + .mode = S_IFREG | 0777, + .nlink = 1, + .filesize = sizeof("ASDF") - 1, + .devminor = 1, + .namesize = sizeof("initramfs_test_csum"), + .csum = 'A' + 'S' + 'D' + 'F', + .fname = "initramfs_test_csum", + .data = "ASDF", + }, { + /* mix csum entry above with no-csum entry below */ + .magic = "070701", + .ino = 2, + .mode = S_IFREG | 0777, + .nlink = 1, + .filesize = sizeof("ASDF") - 1, + .devminor = 1, + .namesize = sizeof("initramfs_test_csum_not_here"), + /* csum ignored */ + .csum = 5555, + .fname = "initramfs_test_csum_not_here", + .data = "ASDF", + } }; + + cpio_srcbuf = kmalloc(8192, GFP_KERNEL); + + len = fill_cpio(c, ARRAY_SIZE(c), cpio_srcbuf); + + err = unpack_to_rootfs(cpio_srcbuf, len); + KUNIT_EXPECT_NULL(test, err); + + KUNIT_EXPECT_EQ(test, init_unlink(c[0].fname), 0); + KUNIT_EXPECT_EQ(test, init_unlink(c[1].fname), 0); + + /* mess up the csum and confirm that unpack fails */ + c[0].csum--; + len = fill_cpio(c, ARRAY_SIZE(c), cpio_srcbuf); + + err = unpack_to_rootfs(cpio_srcbuf, len); + KUNIT_EXPECT_NOT_NULL(test, err); + + /* + * file (with content) is still retained in case of bad-csum abort. + * Perhaps we should change this. + */ + KUNIT_EXPECT_EQ(test, init_unlink(c[0].fname), 0); + KUNIT_EXPECT_EQ(test, init_unlink(c[1].fname), -ENOENT); + kfree(cpio_srcbuf); +} + +static void __init initramfs_test_hardlink(struct kunit *test) +{ + char *err, *cpio_srcbuf; + size_t len; + struct kstat st0, st1; + struct initramfs_test_cpio c[] = { { + .magic = "070701", + .ino = 1, + .mode = S_IFREG | 0777, + .nlink = 2, + .devminor = 1, + .namesize = sizeof("initramfs_test_hardlink"), + .fname = "initramfs_test_hardlink", + }, { + /* hardlink data is present in last archive entry */ + .magic = "070701", + .ino = 1, + .mode = S_IFREG | 0777, + .nlink = 2, + .filesize = sizeof("ASDF") - 1, + .devminor = 1, + .namesize = sizeof("initramfs_test_hardlink_link"), + .fname = "initramfs_test_hardlink_link", + .data = "ASDF", + }, { + /* hardlink hashtable leaks when the archive omits a trailer */ + .magic = "070701", + .namesize = sizeof("TRAILER!!!"), + .fname = "TRAILER!!!", + } }; + + cpio_srcbuf = kmalloc(8192, GFP_KERNEL); + + len = fill_cpio(c, ARRAY_SIZE(c), cpio_srcbuf); + + err = unpack_to_rootfs(cpio_srcbuf, len); + KUNIT_EXPECT_NULL(test, err); + + KUNIT_EXPECT_EQ(test, init_stat(c[0].fname, &st0, 0), 0); + KUNIT_EXPECT_EQ(test, init_stat(c[1].fname, &st1, 0), 0); + KUNIT_EXPECT_EQ(test, st0.ino, st1.ino); + KUNIT_EXPECT_EQ(test, st0.nlink, 2); + KUNIT_EXPECT_EQ(test, st1.nlink, 2); + + KUNIT_EXPECT_EQ(test, init_unlink(c[0].fname), 0); + KUNIT_EXPECT_EQ(test, init_unlink(c[1].fname), 0); + + kfree(cpio_srcbuf); +} + +#define INITRAMFS_TEST_MANY_LIMIT 1000 +#define INITRAMFS_TEST_MANY_PATH_MAX (sizeof("initramfs_test_many-") \ + + sizeof(__stringify(INITRAMFS_TEST_MANY_LIMIT))) +static void __init initramfs_test_many(struct kunit *test) +{ + char *err, *cpio_srcbuf, *p; + size_t len = INITRAMFS_TEST_MANY_LIMIT * + (CPIO_HDRLEN + INITRAMFS_TEST_MANY_PATH_MAX + 3); + char thispath[INITRAMFS_TEST_MANY_PATH_MAX]; + int i; + + p = cpio_srcbuf = kmalloc(len, GFP_KERNEL); + + for (i = 0; i < INITRAMFS_TEST_MANY_LIMIT; i++) { + struct initramfs_test_cpio c = { + .magic = "070701", + .ino = i, + .mode = S_IFREG | 0777, + .nlink = 1, + .devminor = 1, + .fname = thispath, + }; + + c.namesize = 1 + sprintf(thispath, "initramfs_test_many-%d", i); + p += fill_cpio(&c, 1, p); + } + + len = p - cpio_srcbuf; + err = unpack_to_rootfs(cpio_srcbuf, len); + KUNIT_EXPECT_NULL(test, err); + + for (i = 0; i < INITRAMFS_TEST_MANY_LIMIT; i++) { + sprintf(thispath, "initramfs_test_many-%d", i); + KUNIT_EXPECT_EQ(test, init_unlink(thispath), 0); + } + + kfree(cpio_srcbuf); +} + +/* + * The kunit_case/_suite struct cannot be marked as __initdata as this will be + * used in debugfs to retrieve results after test has run. + */ +static struct kunit_case initramfs_test_cases[] = { + KUNIT_CASE(initramfs_test_extract), + KUNIT_CASE(initramfs_test_fname_overrun), + KUNIT_CASE(initramfs_test_data), + KUNIT_CASE(initramfs_test_csum), + KUNIT_CASE(initramfs_test_hardlink), + KUNIT_CASE(initramfs_test_many), + {}, +}; + +static struct kunit_suite initramfs_test_suite = { + .name = "initramfs", + .test_cases = initramfs_test_cases, +}; +kunit_test_init_section_suites(&initramfs_test_suite); + +MODULE_DESCRIPTION("Initramfs KUnit test suite"); +MODULE_LICENSE("GPL v2");
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on akpm-mm/mm-nonmm-unstable] [also build test WARNING on brauner-vfs/vfs.all linus/master v6.12-rc6 next-20241108] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/David-Disseldorp/init-add-ini... base: https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-nonmm-unstable patch link: https://lore.kernel.org/r/20241107002044.16477-3-ddiss%40suse.de patch subject: [PATCH v3 2/9] initramfs_test: kunit tests for initramfs unpacking config: sh-randconfig-r122-20241108 (https://download.01.org/0day-ci/archive/20241109/202411090808.exzPhnlj-lkp@i...) compiler: sh4-linux-gcc (GCC) 14.2.0 reproduce: (https://download.01.org/0day-ci/archive/20241109/202411090808.exzPhnlj-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202411090808.exzPhnlj-lkp@intel.com/
All warnings (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: vmlinux: section mismatch in reference: initramfs_test_cases+0x0 (section: .data) -> set_reset_devices (section: .init.text)
WARNING: modpost: vmlinux: section mismatch in reference: initramfs_test_cases+0x1c (section: .data) -> set_reset_devices (section: .init.text) WARNING: modpost: vmlinux: section mismatch in reference: initramfs_test_cases+0x38 (section: .data) -> set_reset_devices (section: .init.text) WARNING: modpost: vmlinux: section mismatch in reference: initramfs_test_cases+0x54 (section: .data) -> set_reset_devices (section: .init.text) WARNING: modpost: vmlinux: section mismatch in reference: initramfs_test_cases+0x70 (section: .data) -> set_reset_devices (section: .init.text) WARNING: modpost: vmlinux: section mismatch in reference: initramfs_test_cases+0x8c (section: .data) -> set_reset_devices (section: .init.text)
cpio extraction currently does a memcpy to ensure that the archive hex fields are null terminated for simple_strtoul(). simple_strntoul() will allow us to avoid the memcpy.
Signed-off-by: David Disseldorp ddiss@suse.de --- include/linux/kstrtox.h | 1 + lib/vsprintf.c | 7 +++++++ 2 files changed, 8 insertions(+)
diff --git a/include/linux/kstrtox.h b/include/linux/kstrtox.h index 7fcf29a4e0de4..6ea897222af1d 100644 --- a/include/linux/kstrtox.h +++ b/include/linux/kstrtox.h @@ -143,6 +143,7 @@ static inline int __must_check kstrtos32_from_user(const char __user *s, size_t */
extern unsigned long simple_strtoul(const char *,char **,unsigned int); +extern unsigned long simple_strntoul(const char *,char **,unsigned int,size_t); extern long simple_strtol(const char *,char **,unsigned int); extern unsigned long long simple_strtoull(const char *,char **,unsigned int); extern long long simple_strtoll(const char *,char **,unsigned int); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index c5e2ec9303c5d..32eacaae97990 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -114,6 +114,13 @@ unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base) } EXPORT_SYMBOL(simple_strtoul);
+unsigned long simple_strntoul(const char *cp, char **endp, unsigned int base, + size_t max_chars) +{ + return simple_strntoull(cp, endp, base, max_chars); +} +EXPORT_SYMBOL(simple_strntoul); + /** * simple_strtol - convert a string to a signed long * @cp: The start of the string
newc/crc cpio headers contain a bunch of 8-character hexadecimal fields which we convert via simple_strtoul(), following memcpy() into a zero-terminated stack buffer. The new simple_strntoul() helper allows us to pass in max_chars=8 to avoid zero-termination and memcpy().
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index 002e83ae12ac7..6dd3b02c15d7e 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -189,14 +189,11 @@ static __initdata u32 hdr_csum; static void __init parse_header(char *s) { unsigned long parsed[13]; - char buf[9]; int i;
- buf[8] = '\0'; - for (i = 0, s += 6; i < 13; i++, s += 8) { - memcpy(buf, s, 8); - parsed[i] = simple_strtoul(buf, NULL, 16); - } + for (i = 0, s += 6; i < 13; i++, s += 8) + parsed[i] = simple_strntoul(s, NULL, 16, 8); + ino = parsed[0]; mode = parsed[1]; uid = parsed[2];
A (newc/crc) cpio entry with mode.S_IFLNK set carries the symlink target in the cpio data segment, following the padded name_len sized file path. symlink_buf is heap-allocated for staging both file path and symlink target, while name_buf is additionally allocated for staging paths for non-symlink cpio entries.
Separate symlink / non-symlink buffers are unnecessary, so just extend the size of name_buf and use it for both.
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index 6dd3b02c15d7e..59b4a43fa491b 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -250,7 +250,7 @@ static void __init read_into(char *buf, unsigned size, enum state next) } }
-static __initdata char *header_buf, *symlink_buf, *name_buf; +static __initdata char *header_buf, *name_buf;
static int __init do_start(void) { @@ -294,7 +294,7 @@ static int __init do_header(void) if (S_ISLNK(mode)) { if (body_len > PATH_MAX) return 0; - collect = collected = symlink_buf; + collect = collected = name_buf; remains = N_ALIGN(name_len) + body_len; next_state = GotSymlink; state = Collect; @@ -512,10 +512,9 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) static __initdata char msg_buf[64];
header_buf = kmalloc(CPIO_HDRLEN, GFP_KERNEL); - symlink_buf = kmalloc(PATH_MAX + N_ALIGN(PATH_MAX) + 1, GFP_KERNEL); - name_buf = kmalloc(N_ALIGN(PATH_MAX), GFP_KERNEL); - - if (!header_buf || !symlink_buf || !name_buf) + /* 2x PATH_MAX as @name_buf is also used for staging symlink targets */ + name_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL); + if (!header_buf || !name_buf) panic_show_mem("can't allocate buffers");
state = Start; @@ -561,7 +560,6 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) } dir_utime(); kfree(name_buf); - kfree(symlink_buf); kfree(header_buf); return message; }
header_buf is only used in FSM states up to GotHeader, while name_buf is only used in states following GotHeader (Collect is shared, but the collect pointer tracks each buffer). These buffers can therefore be combined into a single cpio_buf, which can be used for both header and file name storage.
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index 59b4a43fa491b..4e2506a4bc76f 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -250,11 +250,11 @@ static void __init read_into(char *buf, unsigned size, enum state next) } }
-static __initdata char *header_buf, *name_buf; +static __initdata char *cpio_buf;
static int __init do_start(void) { - read_into(header_buf, CPIO_HDRLEN, GotHeader); + read_into(cpio_buf, CPIO_HDRLEN, GotHeader); return 0; }
@@ -294,14 +294,14 @@ static int __init do_header(void) if (S_ISLNK(mode)) { if (body_len > PATH_MAX) return 0; - collect = collected = name_buf; + collect = collected = cpio_buf; remains = N_ALIGN(name_len) + body_len; next_state = GotSymlink; state = Collect; return 0; } if (S_ISREG(mode) || !body_len) - read_into(name_buf, N_ALIGN(name_len), GotName); + read_into(cpio_buf, N_ALIGN(name_len), GotName); return 0; }
@@ -511,11 +511,16 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) const char *compress_name; static __initdata char msg_buf[64];
- header_buf = kmalloc(CPIO_HDRLEN, GFP_KERNEL); - /* 2x PATH_MAX as @name_buf is also used for staging symlink targets */ - name_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL); - if (!header_buf || !name_buf) - panic_show_mem("can't allocate buffers"); + /* + * @cpio_buf can be used for staging the 110 byte newc/crc cpio header, + * after which parse_header() converts and stashes fields into + * corresponding types. The same buffer can then be reused for file + * path staging. 2 x PATH_MAX covers any possible symlink target. + */ + BUILD_BUG_ON(CPIO_HDRLEN > N_ALIGN(PATH_MAX) + PATH_MAX + 1); + cpio_buf = kmalloc(N_ALIGN(PATH_MAX) + PATH_MAX + 1, GFP_KERNEL); + if (!cpio_buf) + panic_show_mem("can't allocate buffer");
state = Start; this_header = 0; @@ -559,8 +564,7 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) len -= my_inptr; } dir_utime(); - kfree(name_buf); - kfree(header_buf); + kfree(cpio_buf); return message; }
We already have a nulterm-inclusive, checked name_len for the directory name, so use that instead of calling strlen().
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index 4e2506a4bc76f..c264f136c5281 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -144,9 +144,8 @@ struct dir_entry { char name[]; };
-static void __init dir_add(const char *name, time64_t mtime) +static void __init dir_add(const char *name, size_t nlen, time64_t mtime) { - size_t nlen = strlen(name) + 1; struct dir_entry *de;
de = kmalloc(sizeof(struct dir_entry) + nlen, GFP_KERNEL); @@ -170,7 +169,7 @@ static void __init dir_utime(void) #else static void __init do_utime(char *filename, time64_t mtime) {} static void __init do_utime_path(const struct path *path, time64_t mtime) {} -static void __init dir_add(const char *name, time64_t mtime) {} +static void __init dir_add(const char *name, size_t nlen, time64_t mtime) {} static void __init dir_utime(void) {} #endif
@@ -394,7 +393,7 @@ static int __init do_name(void) init_mkdir(collected, mode); init_chown(collected, uid, gid, 0); init_chmod(collected, mode); - dir_add(collected, mtime); + dir_add(collected, name_len, mtime); } else if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode)) { if (maybe_link() == 0) {
Covered in Documentation/driver-api/early-userspace/buffer-format.rst , initramfs archives can carry an optional "TRAILER!!!" entry which serves as a boundary for collecting and associating hardlinks with matching inode and major / minor device numbers.
Although optional, if hardlinks are found in an archive without a subsequent "TRAILER!!!" entry then the hardlink state hash table is leaked, e.g. unfixed kernel, with initramfs_test.c hunk applied only: unreferenced object 0xffff9405408cc000 (size 8192): comm "kunit_try_catch", pid 53, jiffies 4294892519 hex dump (first 32 bytes): 01 00 00 00 01 00 00 00 00 00 00 00 ff 81 00 00 ................ 00 00 00 00 00 00 00 00 69 6e 69 74 72 61 6d 66 ........initramf backtrace (crc a9fb0ee0): [<0000000066739faa>] __kmalloc_cache_noprof+0x11d/0x250 [<00000000fc755219>] maybe_link.part.5+0xbc/0x120 [<000000000526a128>] do_name+0xce/0x2f0 [<00000000145c1048>] write_buffer+0x22/0x40 [<000000003f0b4f32>] unpack_to_rootfs+0xf9/0x2a0 [<00000000d6f7e5af>] initramfs_test_hardlink+0xe3/0x3f0 [<0000000014fde8d6>] kunit_try_run_case+0x5f/0x130 [<00000000dc9dafc5>] kunit_generic_run_threadfn_adapter+0x18/0x30 [<000000001076c239>] kthread+0xc8/0x100 [<00000000d939f1c1>] ret_from_fork+0x2b/0x40 [<00000000f848ad1a>] ret_from_fork_asm+0x1a/0x30
Fix this by calling free_hash() after initramfs buffer processing in unpack_to_rootfs(). An extra hardlink_seen global is added as an optimization to avoid walking the 32 entry hash array unnecessarily. The expectation is that a "TRAILER!!!" entry will normally be present, and initramfs hardlinks are uncommon.
There is one user facing side-effect of this fix: hardlinks can currently be associated across built-in and external initramfs archives, *if* the built-in initramfs archive lacks a "TRAILER!!!" terminator. I'd consider this cross-archive association broken, but perhaps it's used.
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 7 ++++++- init/initramfs_test.c | 5 ----- 2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index c264f136c5281..99f3cac10d392 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -76,6 +76,7 @@ static __initdata struct hash { struct hash *next; char name[N_ALIGN(PATH_MAX)]; } *head[32]; +static __initdata bool hardlink_seen;
static inline int hash(int major, int minor, int ino) { @@ -109,19 +110,21 @@ static char __init *find_link(int major, int minor, int ino, strcpy(q->name, name); q->next = NULL; *p = q; + hardlink_seen = true; return NULL; }
static void __init free_hash(void) { struct hash **p, *q; - for (p = head; p < head + 32; p++) { + for (p = head; hardlink_seen && p < head + 32; p++) { while (*p) { q = *p; *p = q->next; kfree(q); } } + hardlink_seen = false; }
#ifdef CONFIG_INITRAMFS_PRESERVE_MTIME @@ -563,6 +566,8 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) len -= my_inptr; } dir_utime(); + /* free any hardlink state collected without optional TRAILER!!! */ + free_hash(); kfree(cpio_buf); return message; } diff --git a/init/initramfs_test.c b/init/initramfs_test.c index 84b21f465bc3d..a2930c70cc817 100644 --- a/init/initramfs_test.c +++ b/init/initramfs_test.c @@ -319,11 +319,6 @@ static void __init initramfs_test_hardlink(struct kunit *test) .namesize = sizeof("initramfs_test_hardlink_link"), .fname = "initramfs_test_hardlink_link", .data = "ASDF", - }, { - /* hardlink hashtable leaks when the archive omits a trailer */ - .magic = "070701", - .namesize = sizeof("TRAILER!!!"), - .fname = "TRAILER!!!", } };
cpio_srcbuf = kmalloc(8192, GFP_KERNEL);
On Thu, 7 Nov 2024 11:17:27 +1100, David Disseldorp wrote:
Covered in Documentation/driver-api/early-userspace/buffer-format.rst , initramfs archives can carry an optional "TRAILER!!!" entry which serves as a boundary for collecting and associating hardlinks with matching inode and major / minor device numbers.
Although optional, if hardlinks are found in an archive without a subsequent "TRAILER!!!" entry then the hardlink state hash table is leaked
One further leak is possible if extraction ends prior to fput(wfile) in CopyFile state, e.g. due to lack of data:
nilchar="\0" data="123456789ABCDEF" magic="070701" ino=1 mode=$(( 0100777 )) uid=0 gid=0 nlink=1 mtime=1 filesize=$(( ${#data} + 20 )) # too much devmajor=0 devminor=1 rdevmajor=0 rdevminor=0 csum=0 fname="initramfs_test_archive_overrun" namelen=$(( ${#fname} + 1 )) # plus one to account for terminator
printf "%s%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%08x%s" \ $magic $ino $mode $uid $gid $nlink $mtime $filesize \ $devmajor $devminor $rdevmajor $rdevminor $namelen $csum $fname
termpadlen=$(( 1 + ((4 - ((110 + $namelen) & 3)) % 4) )) printf "%.s${nilchar}" $(seq 1 $termpadlen) # $filesize reaches 20 bytes beyond end of data printf "%s" "$data"
bash data_repro.sh|gzip >> initramfs
unreferenced object 0xffff8fdb0192e000 (size 176): comm "kworker/u8:0", pid 11, jiffies 4294892503 hex dump (first 32 bytes): 01 00 00 00 00 00 00 00 00 00 00 00 1e 80 5d 00 ..............]. 80 7d a1 a7 ff ff ff ff 10 b1 2f 02 db 8f ff ff .}......../..... backtrace (crc 807bd733): [<00000000e68e8b32>] kmem_cache_alloc_noprof+0x11e/0x260 [<00000000a6f24fcd>] alloc_empty_file+0x45/0x120 [<00000000130beec8>] path_openat+0x2f/0xf30 [<0000000024613ad7>] do_filp_open+0xa7/0x110 [<000000005f4f0158>] file_open_name+0x118/0x180 [<0000000003ed573f>] filp_open+0x27/0x50 [<0000000091ec9e44>] do_name+0xc4/0x2b0 [<000000008e084ec8>] write_buffer+0x22/0x40 [<000000002ea2ff4b>] flush_buffer+0x2f/0x90 [<000000009085f8b5>] gunzip+0x25a/0x310 [<000000000c1c83c3>] unpack_to_rootfs+0x176/0x2a0 [<00000000c966fda5>] do_populate_rootfs+0x6a/0x180 [<0000000051fb877d>] async_run_entry_fn+0x31/0x120 [<00000000a3ee305f>] process_scheduled_works+0xbe/0x310 [<0000000083c835bb>] worker_thread+0x100/0x240 [<000000006ea2f0b3>] kthread+0xc8/0x100
Not sure whether others are interested in seeing these kinds of leak-on-malformed-archive bugs fixed, but I'll send through a v4 with a fix + unit test for it.
The dynamic error message printed if CONFIG_RD_$ALG compression support is missing needn't be propagated up to the caller via a static buffer. Print it immediately via pr_err() and set @message to a const string to flag error.
Before: text data bss dec hex filename 7695 1102 8 8805 2265 ./init/initramfs.o
After: text data bss dec hex filename 7683 1006 8 8697 21f9 ./init/initramfs.o
Signed-off-by: David Disseldorp ddiss@suse.de --- init/initramfs.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/init/initramfs.c b/init/initramfs.c index 99f3cac10d392..f946b7680867b 100644 --- a/init/initramfs.c +++ b/init/initramfs.c @@ -511,7 +511,6 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) long written; decompress_fn decompress; const char *compress_name; - static __initdata char msg_buf[64];
/* * @cpio_buf can be used for staging the 110 byte newc/crc cpio header, @@ -551,12 +550,9 @@ char * __init unpack_to_rootfs(char *buf, unsigned long len) if (res) error("decompressor failed"); } else if (compress_name) { - if (!message) { - snprintf(msg_buf, sizeof msg_buf, - "compression method %s not configured", - compress_name); - message = msg_buf; - } + pr_err("compression method %s not configured\n", + compress_name); + error("decompressor failed"); } else error("invalid magic at start of compressed archive"); if (state != Reset)
linux-kselftest-mirror@lists.linaro.org
-
David Disseldorp -
kernel test robot