Re: [PATCH v5 2/4] initramfs: add file metadata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] initramfs: pointer arithmetic overflow in do_parse_metadata() bounds checks
- [Medium] initramfs: do_setxattrs() rejects POSIX ACLs and skips xattr size limits
--

commit a61596ea347667fb16df9d8f6201e38b916f632c
Author: Mimi Zohar <zohar@linux.vnet.ibm.com>

initramfs: add file metadata

This commit introduces do_parse_metadata() and do_setxattrs() to parse
metadata records from a buffer and restore extended attributes onto files
in the initramfs.

> diff --git a/init/initramfs.c b/init/initramfs.c
> index ebddde9c8f0d9..36254dba78226 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c

[ ... ]

> @@ -365,6 +368,90 @@ static int __init maybe_link(void)
>  	return 0;
>  }
>  
> +static int __init do_setxattrs(char *pathname, char *buf, size_t size)
> +{
> +	struct path path;
> +	char *xattr_name, *xattr_value;
> +	uint32_t xattr_name_size, xattr_value_size;
> +	int ret;
> +
> +	xattr_name = buf;
> +	xattr_name_size = strnlen(xattr_name, size);
> +	if (xattr_name_size == size) {
> +		error("malformed xattrs");
> +		return -EINVAL;
> +	}
> +
> +	xattr_value = xattr_name + xattr_name_size + 1;
> +	xattr_value_size = buf + size - xattr_value;
> +
> +	ret = kern_path(pathname, 0, &path);
> +	if (!ret) {
> +		ret = vfs_setxattr(&nop_mnt_idmap, path.dentry, xattr_name, xattr_value,
> +				   xattr_value_size, 0);
> +
> +		path_put(&path);
> +	}

[Severity: Medium]
What happens when a metadata record in the initramfs contains POSIX ACL
extended attributes (system.posix_acl_access or system.posix_acl_default),
or when xattr_name_size or xattr_value_size exceeds the standard VFS
limits?

In fs/xattr.c, the normal setxattr path (do_setxattr()) checks
is_posix_acl_xattr() and dispatches POSIX ACLs to do_set_acl(), whereas
vfs_setxattr() -> __vfs_setxattr_locked() -> __vfs_setxattr_noperm() ->
__vfs_setxattr() explicitly rejects them:

    if (is_posix_acl_xattr(name))
        return -EOPNOTSUPP;

Because gen_init_cpio collects all extended attributes via llistxattr()
and lgetxattr(), any POSIX ACLs packed into the archive will fail to be
restored on rootfs (when CONFIG_TMPFS_POSIX_ACL=y) with -EOPNOTSUPP.

In addition, unlike import_xattr_name() and setxattr_copy(), which enforce
a non-empty name up to XATTR_NAME_MAX (255) and a value up to
XATTR_SIZE_MAX (65536), do_setxattrs() only checks xattr_name_size == size
before calling vfs_setxattr(). On tmpfs/rootfs, shmem_xattr_handler_set() ->
simple_xattr_set() will allocate and insert oversized names or values into
the inode xattr list, after which userspace getxattr(2) and removexattr(2)
calls fail with -E2BIG or -ERANGE.

Should do_setxattrs() validate xattr_name_size (1..XATTR_NAME_MAX) and
xattr_value_size (<= XATTR_SIZE_MAX), and dispatch POSIX ACL xattrs via
vfs_set_acl() (or use the VFS helper that handles both)?

> +
> +	pr_debug("%s: %s size: %u (ret: %d)\n", pathname, xattr_name,
> +		 xattr_value_size, ret);
> +	print_hex_dump_debug("xattr value: ", DUMP_PREFIX_OFFSET,
> +			     16, 1, xattr_value, xattr_value_size, false);
> +
> +	return ret;
> +}
> +
> +static int __init __maybe_unused do_parse_metadata(char *pathname)
> +{
> +	char *buf = metadata_buf;
> +	char *bufend = metadata_buf + metadata_len;
> +	struct metadata_hdr *hdr;
> +	char str[sizeof(hdr->c_size) + 1];
> +	uint32_t entry_size;
> +
> +	if (!metadata_len)
> +		return 0;
> +
> +	str[sizeof(hdr->c_size)] = 0;
> +
> +	while (buf < bufend) {
> +		int ret;
> +
> +		if (buf + sizeof(*hdr) > bufend) {
> +			error("malformed metadata");
> +			break;
> +		}
> +
> +		hdr = (struct metadata_hdr *)buf;
> +		if (hdr->c_version != 1) {
> +			pr_debug("Unsupported header version\n");
> +			break;
> +		}
> +
> +		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;
> +		default:
> +			pr_debug("Unsupported metadata type\n");
> +			break;
> +		}
> +
> +		buf += entry_size;
> +	}

[Severity: High]
Can adding entry_size to buf in do_parse_metadata() overflow pointer
arithmetic and bypass the bounds check?

kstrtou32(str, 16, &entry_size) parses an 8-character hex value from
hdr->c_size into a 32-bit unsigned integer up to 0xffffffff. On 32-bit
architectures (where kernel virtual addresses reside in the upper address
space, such as >= 0xc0000000 or >= 0x80000000), or on 64-bit architectures
if metadata_buf lies in the top 4 GiB of the address space, adding a large
entry_size to buf wraps around the address space to an address lower than
bufend.

When that wraparound occurs, both buf + entry_size > bufend and
entry_size < sizeof(*hdr) evaluate to false, passing a huge out-of-bounds
length (entry_size - sizeof(*hdr)) to do_setxattrs() for strnlen(),
vfs_setxattr(), and print_hex_dump_debug(), and then advancing buf to the
wrapped address below bufend.

Since the loop condition while (buf < bufend) guarantees bufend > buf,
should the bounds checks compare sizes against bufend - buf instead, i.e.
bufend - buf < sizeof(*hdr) and entry_size > bufend - buf?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030210944.2143778-1-dannyhu@arista.com?part=2