Re: [PATCH v3] environment: support reading and writing of efivar

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Jonas,

On 20.03.25 10:22, Jonas Licht wrote:
> scripts/bareboxenv: skip file creation to allow reading the destination
> file, when existing.
> 
> In envfs_load before reading the superblock, we check if our superblock
> magic exists after an EFIVAR_ATTR_SIZE offset. This is necessary because
> linux kernel adds four byte variable attributes in front of each efi
> variable in sysfs. When the magic is found after EFIVAR_ATTR_SIZE, we
> skip reading the four bytes variable attributes.
> For envfs_save detection of an efi var is done the same way. But we need
> to keep the attributes, as they are required for writing as well.
> 
> In order to write to efivars, you still need to remove immutable flag
> from the file with 'chattr -i' .

Sorry for taking so long to get back to you.
I agree this is a problem we want to fix, but I wasn't overly happy with
the complexity the fix introduced.

How about this alternative:

- We define in scripts/bareboxenv.c following function:
    
bool is_efivarfs_fd(int fd)
{
	struct statfs env_statfs;
	int ret;

	ret = fstatvfs(fd, &env_statfs);
	if (ret)
		return false;

	return bundle_statfs.f_type == EFIVARFS_MAGIC;
}

In common/environment.c, in the first #ifdef __BAREBOX__ branch,
we #define is_efivarfs_fd(fd) false.

- Instead of checking the magic, we just call is_efivarfs_fd and
then read the 4 bytes

- We get rid of the #ifdefs and just let the compiler drop dead code
  when building for __BAREBOX__.

- As we limit the effect to bareboxenv as hosted tool, we don't need
  to check for CONFIG_EFI either

What do you think?

Some minor comments below. 

> Skip the detect code when __BAREBOX__ is defined as well.
> I've noticed that the first pread in detect_efi_vars never comes back
> in __BAREBOX__ , when the efivar file don't exists, but doing this on a
> file at /tmp works.

Oh, so that's a bug in the barebox efivarfs implementation?

> +#include <autoconf.h>

autoconf.h should be included by the buildsystem (-include option), but
with suggested changes above we can avoid it altogether.

> +	void *buf = NULL, *wbuf = NULL, *buf_with_efi;
>  	struct envfs_entry *env;
>  	const char *defenv_path = default_environment_path_get();
>  
> @@ -342,7 +380,9 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  		}
>  	}
>  
> -	buf = xzalloc(size + sizeof(struct envfs_super));
> +	buf_with_efi = xzalloc(size + sizeof(struct envfs_super) +
> +	EFIVAR_ATTR_SIZE);

Alignment is wonky, but you can just keep it a single line here.

> +	buf = buf_with_efi + EFIVAR_ATTR_SIZE;
>  	data.writep = buf + sizeof(struct envfs_super);
>  
>  	super = buf;
> @@ -370,7 +410,7 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  	super->crc = ENVFS_32(crc32(0, buf + sizeof(struct envfs_super), size));
>  	super->sb_crc = ENVFS_32(crc32(0, buf, sizeof(struct envfs_super) - 4));
>  
> -	envfd = open(filename, O_WRONLY | O_CREAT, S_IRUSR | S_IWUSR);
> +	envfd = open(filename, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
>  	if (envfd < 0) {
>  		printf("could not open %s: %m\n", filename);
>  		ret = -errno;
> @@ -385,6 +425,12 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  		goto out;
>  	}
>  
> +	/* check if we writing efi vars */
> +	ret = detect_efi_vars(envfd, buf_with_efi, &wbuf, &size);
> +	if (ret != 0) {
> +		goto out;
> +	}

No braces for single line if clauses.

> +
>  	ret = erase(envfd, ERASE_SIZE_ALL, 0, ERASE_TO_WRITE);
>  
>  	/* ENOSYS and EOPNOTSUPP aren't errors here, many devices don't need it */
> @@ -395,12 +441,11 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  
>  	size += sizeof(struct envfs_super);
>  
> -	wbuf = buf;
> -
>  	while (size) {
>  		ssize_t now = write(envfd, wbuf, size);
>  		if (now < 0) {
>  			ret = -errno;
> +			perror("write");
>  			goto out;
>  		}
>  
> @@ -418,14 +463,14 @@ int envfs_save(const char *filename, const char *dirname, unsigned flags)
>  
>  	ret = 0;
>  
> -#ifdef CONFIG_NVVAR
> +#if defined CONFIG_NVVAR && defined __BAREBOX__
>  	if (defenv_path && !strcmp(filename, defenv_path))
>  	    nv_var_set_clean();
>  #endif
>  out:
>  	close(envfd);
>  out1:
> -	free(buf);
> +	free(buf_with_efi);
>  #ifdef __BAREBOX__
>  	unlink_recursive(TMPDIR, NULL);
>  #endif
> @@ -449,6 +494,9 @@ int envfs_load(const char *filename, const char *dir, unsigned flags)
>  	int envfd;
>  	int ret = 0;
>  	size_t size, rsize;
> +#if defined CONFIG_EFI && !defined __BAREBOX__
> +	uint32_t magic;
> +#endif

Won't be needed with suggested rework.

>  
>  	if (!filename)
>  		filename = default_environment_path_get();
> @@ -466,6 +514,28 @@ int envfs_load(const char *filename, const char *dir, unsigned flags)
>  		return -1;
>  	}
>  
> +#if defined CONFIG_EFI && !defined __BAREBOX__
> +	/* check if we reading efi vars */
> +	ret = pread(envfd, &magic, sizeof(uint32_t),
> +		    EFIVAR_ATTR_SIZE);

Just make it a single line. With suggested changes above this becomes
a normal read.

Thanks,
Ahmad


> +	if (ret < sizeof(u_int32_t)) {
> +		perror("read");
> +		ret = -errno;
> +		goto out;
> +	}
> +
> +	if (ENVFS_32(magic) == ENVFS_MAGIC) {
> +		pr_info("Assuming EFI variable. Skip attributes\n");
> +		ret = read(envfd, &magic,
> +			   sizeof(uint32_t)); // simply reuse the memory
> +		if (ret < sizeof(uint32_t)) {
> +			perror("read");
> +			ret = -errno;
> +			goto out;
> +		}
> +	}
> +#endif
> +
>  	/* read superblock */
>  	ret = read(envfd, &super, sizeof(struct envfs_super));
>  	if ( ret < sizeof(struct envfs_super)) {
> diff --git a/scripts/Makefile b/scripts/Makefile
> index 6d89af7d4f..57d8ddb54d 100644
> --- a/scripts/Makefile
> +++ b/scripts/Makefile
> @@ -30,6 +30,7 @@ hostprogs-always-$(CONFIG_RK_IMAGE)			+= rkimage
>  HOSTCFLAGS_rkimage.o = `$(PKG_CONFIG) --cflags openssl`
>  HOSTLDLIBS_rkimage = `$(PKG_CONFIG) --libs openssl`
>  KBUILD_HOSTCFLAGS += -I$(srctree)/scripts/include/
> +KBUILD_HOSTCFLAGS += -I$(objtree)/include/generated/
>  HOSTCFLAGS_mxsimage.o = `$(PKG_CONFIG) --cflags openssl`
>  HOSTLDLIBS_mxsimage  = `$(PKG_CONFIG) --libs openssl`
>  HOSTCFLAGS_omap3-usb-loader.o = `$(PKG_CONFIG) --cflags libusb-1.0`
> @@ -57,6 +58,7 @@ omap4_usbboot-target-userccflags += `$(CROSS_PKG_CONFIG) --cflags libusb-1.0`
>  omap4_usbboot-target-userldlibs += -lpthread `$(CROSS_PKG_CONFIG) --libs libusb-1.0`
>  rk-usb-loader-target-userccflags += `$(CROSS_PKG_CONFIG) --cflags libusb-1.0`
>  rk-usb-loader-target-userldlibs += `$(CROSS_PKG_CONFIG) --libs libusb-1.0`
> +bareboxenv-target-userccflags += -I$(objtree)/include/generated/
>  
>  userccflags += -I $(srctree)/$(src)/include -isystem $(srctree)/scripts/include
>  
> diff --git a/scripts/bareboxenv.c b/scripts/bareboxenv.c
> index e954447015..6b9b8d90c4 100644
> --- a/scripts/bareboxenv.c
> +++ b/scripts/bareboxenv.c
> @@ -117,7 +117,7 @@ static void usage(char *prgname)
>  int main(int argc, char *argv[])
>  {
>  	int opt;
> -	int save = 0, load = 0, pad = 0, err = 0, fd;
> +	int save = 0, load = 0, pad = 0, err = 0;
>  	char *filename = NULL, *dirname = NULL;
>  	unsigned envfs_flags = 0;
>  	int verbose = 0;
> @@ -156,15 +156,6 @@ int main(int argc, char *argv[])
>  		exit(1);
>  	}
>  
> -	if (save) {
> -		fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0644);
> -		if (fd < 0) {
> -			perror("open");
> -			exit(1);
> -		}
> -		close(fd);
> -	}
> -
>  	if (save && pad) {
>  		if (truncate(filename, pad)) {
>  			perror("truncate");


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |