Re: [PATCH v5 4/4] gen_init_cpio: add support for file metadata

[sashiko-bot@kernel.org]

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

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