Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] initramfs: NULL path passed to llistxattr() in write_xattrs() - [Low] initramfs: undefined __packed macro in host tool usr/gen_init_cpio.c --
commit f793fe8aaa70c6c2c7baf3bf7456e36e56bc7fcf Author: Roberto Sassu <[email protected]> gen_init_cpio: add support for file metadata This commit adds support for including extended attribute (TYPE_XATTR) file metadata in initramfs cpio archives as separate METADATA!!! entries when configured via CONFIG_INITRAMFS_FILE_METADATA. > diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c > index b7296edc6626f..f07be54c9219e 100644 > --- a/usr/gen_init_cpio.c > +++ b/usr/gen_init_cpio.c > @@ -6,6 +6,7 @@ > #include <stdbool.h> > #include <sys/types.h> > #include <sys/stat.h> > +#include <sys/xattr.h> > #include <string.h> > #include <unistd.h> > #include <time.h> > @@ -13,6 +14,7 @@ > #include <errno.h> > #include <ctype.h> > #include <limits.h> > +#include "../include/linux/initramfs.h" [Severity: Low] Since usr/gen_init_cpio.c is compiled as a host userspace program with HOSTCC and standard libc headers, is the kernel __packed macro defined when including ../include/linux/initramfs.h? In include/linux/initramfs.h: struct metadata_hdr { char c_size[8]; /* total size including c_size field */ char c_version; /* header version */ char c_type; /* metadata type */ char c_metadata[]; /* metadata */ } __packed; Without a definition for __packed, the host compiler leaves struct metadata_hdr unpacked and instead parses __packed as a tentative definition of a global variable named __packed of type struct metadata_hdr. Should __packed be defined before including initramfs.h (or compiler_attributes.h included)? > @@ -37,12 +39,126 @@ static bool do_file_mtime; [ ... ] > +static int write_xattrs(const char *path) > +{ > + struct metadata_hdr hdr = { .c_version = 1, .c_type = TYPE_XATTR }; > + char str[sizeof(hdr.c_size) + 1]; > + char *xattr_list, *list_ptr; > + char *xattr_value = NULL; > + ssize_t list_len, name_len, value_len, len; > + int ret = -EINVAL; > + > + if (metadata_fd < 0) > + return 0; > + > + if (path == metadata_path) > + return 0; > + > + list_len = llistxattr(path, NULL, 0); [Severity: Medium] Can path be NULL when passed to llistxattr() in write_xattrs()? For non-regular cpio list entries (slink, dir, pipe, sock, and nod), the source location field is optional and is omitted in standard list files such as usr/default_cpio_list. When location is omitted, cpio_mkslink_line(), cpio_mkgeneric_line(), and cpio_mknod_line() pass NULL as location: cpio_mkgeneric_line() cpio_mkgeneric(name, nfields == 5 ? cpio_replace_env(location) : NULL, mode, uid, gid) write_xattrs(location) When gen_init_cpio is invoked with -e xattr, metadata_fd is valid and write_xattrs() only checks if (metadata_fd < 0) and if (path == metadata_path) before calling llistxattr(NULL, NULL, 0). Passing a NULL path to llistxattr() violates the glibc nonnull attribute on the first parameter (triggering UBSan failures) and can cause a segmentation fault under LD_PRELOAD environments such as fakeroot or pseudo that dereference the path string in userspace. Should write_xattrs() also return 0 if !path? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
