Re: [PATCH v5 3/4] initramfs: read metadata from special file METADATA!!!

[sashiko-bot@kernel.org]

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 <roberto.sassu@huawei.com>

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/20261030210944.2143778-1-dannyhu@arista.com?part=3