| Message ID | 20241107002044.16477-10-ddiss@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | initramfs: kunit tests and cleanups | expand |
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.
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);
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(-)