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 <[email protected]> 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/[email protected]?part=2
