Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] initramfs: METADATA!!! extracted as real file in rootfs by do_name() - [High] initramfs: stale metadata flag in do_name() when CopyFile is skipped - [High] initramfs: uninitialized or stale previous_name_buf in do_parse_metadata() - [High] initramfs: pointer overflow on entry_size check in do_parse_metadata() --
commit 5bc33bcd28bade616f8daa53d99ada76540074e5 Author: Roberto Sassu <[email protected]> initramfs: read metadata from special file METADATA!!! This commit adds support for extracting file metadata from special CPIO archive entries named METADATA!!! that immediately follow the target file. When a METADATA!!! entry is encountered, its contents are buffered in do_process_metadata() and passed to do_parse_metadata() to apply extended attributes to the preceding pathname. > diff --git a/init/initramfs.c b/init/initramfs.c > index 36254dba78226..a1744cc9523c2 100644 > --- a/init/initramfs.c > +++ b/init/initramfs.c [ ... ] > @@ -471,6 +472,10 @@ static int __init do_name(void) > if (strcmp(collected, "TRAILER!!!") == 0) { > free_hash(); > return 0; > + } else if (strcmp(collected, METADATA_FILENAME) == 0) { > + metadata = 1; > + } else { > + memcpy(previous_name_buf, collected, strlen(collected) + 1); > } > clean_path(collected, mode); > if (S_ISREG(mode)) { [Severity: High] When collected matches METADATA_FILENAME in do_name(), metadata is set to 1, but execution then falls through into clean_path(), maybe_link(), and filp_open(): init/initramfs.c:do_name() { ... if (S_ISREG(mode)) { int ml = maybe_link(); if (ml >= 0) { int openflags = O_WRONLY|O_CREAT|O_LARGEFILE; if (ml != 1) openflags |= O_TRUNC; wfile = filp_open(collected, openflags, mode); ... state = CopyFile; } } } In do_copy(), xwrite() writes the raw metadata stream into wfile, and neither do_copy() nor unpack_to_rootfs() unlinks METADATA_FILENAME afterwards. Also, if multiple METADATA!!! entries share the same inode number with nlink >= 2, maybe_link() calls clean_path("METADATA!!!", 0) to unlink the file and then calls init_link("METADATA!!!", "METADATA!!!"), which fails with -ENOENT and returns -1. Should METADATA!!! entries be consumed in memory without creating and leaving behind a /METADATA!!! file in rootfs? [Severity: High] What happens if a METADATA_FILENAME entry does not transition state to CopyFile in do_name()? In do_name(), metadata = 1 is set before checking S_ISREG(mode), maybe_link(), and filp_open(), while metadata is only reset to 0 inside do_process_metadata() (called from do_copy()) or at the end of unpack_to_rootfs(). If an entry named METADATA!!! is not a regular file, or if maybe_link() returns -1, or if filp_open() returns an error, do_name() returns with state left as SkipIt and metadata still set to 1. When the next regular file in the archive is processed by do_name(), the else branch updates previous_name_buf without clearing metadata, causing do_copy() to pass that regular file's contents to do_process_metadata() as metadata for itself. Should metadata be reset at the start of do_name() or only set once the entry is ready to be processed? > @@ -508,11 +513,47 @@ static int __init do_name(void) > return 0; > } > > +static int __init do_process_metadata(char *buf, int len, bool last) > +{ > + int ret = 0; > + > + if (!metadata_buf) { > + metadata_buf_ptr = metadata_buf = kmalloc(body_len, GFP_KERNEL); > + if (!metadata_buf_ptr) { > + ret = -ENOMEM; > + goto out; > + } > + > + metadata_len = body_len; > + } > + > + if (metadata_buf_ptr + len > metadata_buf + metadata_len) { > + ret = -EINVAL; > + goto out; > + } > + > + memcpy(metadata_buf_ptr, buf, len); > + metadata_buf_ptr += len; > + > + if (last) > + do_parse_metadata(previous_name_buf); [Severity: High] When do_process_metadata() calls do_parse_metadata(), can the entry_size bounds check wrap around on 32-bit architectures? Looking at do_parse_metadata(): init/initramfs.c:do_parse_metadata() { ... memcpy(str, hdr->c_size, sizeof(hdr->c_size)); ret = kstrtou32(str, 16, &entry_size); if (ret || buf + entry_size > bufend || entry_size < sizeof(*hdr)) { error("malformed xattrs"); break; } switch (hdr->c_type) { case TYPE_XATTR: do_setxattrs(pathname, buf + sizeof(*hdr), entry_size - sizeof(*hdr)); break; ... } Because entry_size is a 32-bit unsigned integer parsed from the 8-byte hex hdr->c_size field, a large value such as 0xffffffff on a 32-bit system can cause buf + entry_size to wrap around the address space to buf - 1, comparing less than or equal to bufend. That passes entry_size - sizeof(*hdr) to do_setxattrs(), reading past the end of metadata_buf in strnlen() and vfs_setxattr(). Should do_parse_metadata() check entry_size > bufend - buf instead of buf + entry_size > bufend? > +out: > + if (ret < 0 || last) { > + kfree(metadata_buf); > + metadata_buf = NULL; > + metadata = 0; > + } > + > + return ret; > +} > + > static int __init do_copy(void) > { > if (byte_count >= body_len) { > if (xwrite(wfile, victim, body_len, &wfile_pos) != body_len) > error("write error"); > + if (metadata) > + do_process_metadata(victim, body_len, true); > > do_utime_path(&wfile->f_path, mtime); > fput(wfile); [Severity: High] In do_copy(), do_process_metadata(victim, body_len, true) is called before the data checksum verification: if (csum_present && io_csum != hdr_csum) error("bad data checksum"); In 070702 archives with csum_present, doesn't this parse and apply the xattrs via vfs_setxattr() before verifying that io_csum matches hdr_csum? [ ... ] > @@ -614,6 +658,7 @@ char * __init unpack_to_rootfs(char *buf, unsigned long > len) > char header[CPIO_HDRLEN]; > char symlink[PATH_MAX + N_ALIGN(PATH_MAX) + 1]; > char name[N_ALIGN(PATH_MAX)]; > + char prev_name[PATH_MAX + N_ALIGN(PATH_MAX) + 1]; > } *bufs = kmalloc_obj(*bufs); > > if (!bufs) > @@ -622,6 +667,7 @@ char * __init unpack_to_rootfs(char *buf, unsigned long > len) > header_buf = bufs->header; > symlink_buf = bufs->symlink; > name_buf = bufs->name; > + previous_name_buf = bufs->prev_name; [Severity: High] Can do_process_metadata() pass an uninitialized or stale previous_name_buf to do_parse_metadata()? In unpack_to_rootfs(), bufs is allocated with kmalloc_obj(*bufs) without initializing bufs->prev_name. If METADATA!!! is the first entry in the archive (or is preceded only by TRAILER!!! or entries skipped in do_header()), do_name() sets metadata = 1 without writing to previous_name_buf. When do_process_metadata() then calls: do_parse_metadata(previous_name_buf) -> do_setxattrs() -> kern_path(pathname, 0, &path) kern_path() and pr_debug() read uninitialized memory from bufs->prev_name and can read past the end of the slab allocation if no NUL terminator is present. Similarly, if the entry immediately before METADATA!!! is skipped in do_header() (for example when name_len > PATH_MAX, S_ISLNK(mode) with body_len > PATH_MAX, or a non-regular non-symlink entry has body_len > 0) or fails to be created in do_name(), previous_name_buf is not cleared and still holds the pathname of an earlier file in the archive, so the metadata would be applied to the wrong file. Should previous_name_buf be initialized (and cleared whenever an entry is skipped or fails, or after metadata is consumed), and checked before parsing metadata? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
