Re: [PATCH v2 5/9] efi_loader: use short-form DP for load options

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Sat, Mar 19, 2022 at 10:11:44AM +0100, Heinrich Schuchardt wrote:
> From: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> The GUID of partitions is sufficient for identification and will stay
> constant in the lifetime of a boot option. The preceding path of the
> device-path may change due to changes in the enumeration of devices.
> Therefore it is preferable to use the short-form of device-paths in load
> options. Adjust the 'efidebug boot add' command accordingly.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	support both short and long form device paths
> 	split off exporting efi_dp_shorten() into a separate patch
> ---
>  cmd/efidebug.c | 70 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 22 deletions(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 401d13cc4c..51e2850d21 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -734,20 +734,20 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
>  }
>  
>  /**
> - * create_initrd_dp() - Create a special device for our Boot### option
> - *
> - * @dev:	Device
> - * @part:	Disk partition
> - * @file:	Filename
> - * Return:	Pointer to the device path or ERR_PTR
> + * create_initrd_dp() - create a special device for our Boot### option
>   *
> + * @dev:	device
> + * @part:	disk partition
> + * @file:	filename
> + * @shortform:	create short form device path
> + * Return:	pointer to the device path or ERR_PTR
>   */
>  static
>  struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
> -					 const char *file)
> +					 const char *file, int shortform)
>  
>  {
> -	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
> +	struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp = NULL;
>  	struct efi_device_path *initrd_dp = NULL;
>  	efi_status_t ret;
>  	const struct efi_initrd_dp id_dp = {
> @@ -771,9 +771,13 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part,
>  		printf("Cannot create device path for \"%s %s\"\n", part, file);
>  		goto out;
>  	}
> +	if (shortform)
> +		short_fp = efi_dp_shorten(tmp_fp);
> +	if (!short_fp)
> +		short_fp = tmp_fp;
>  
>  	initrd_dp = efi_dp_append((const struct efi_device_path *)&id_dp,
> -				  tmp_fp);
> +				  short_fp);
>  
>  out:
>  	efi_free_pool(tmp_dp);
> @@ -807,6 +811,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	size_t label_len, label_len16;
>  	u16 *label;
>  	struct efi_device_path *device_path = NULL, *file_path = NULL;
> +	struct efi_device_path *fp_free = NULL;
>  	struct efi_device_path *final_fp = NULL;
>  	struct efi_device_path *initrd_dp = NULL;
>  	struct efi_load_option lo;
> @@ -826,7 +831,18 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  	argc--;
>  	argv++; /* 'add' */
>  	for (; argc > 0; argc--, argv++) {
> -		if (!strcmp(argv[0], "-b")) {
> +		int shortform;
> +
> +		if (*argv[0] != '-' || strlen(argv[0]) != 2) {
> +				r = CMD_RET_USAGE;
> +				goto out;
> +		}
> +		shortform = 0;
> +		switch (argv[0][1]) {
> +		case 'b':
> +			shortform = 1;
> +			/* fallthrough */
> +		case 'B':
>  			if (argc <  5 || lo.label) {
>  				r = CMD_RET_USAGE;
>  				goto out;
> @@ -849,24 +865,33 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  
>  			/* file path */
>  			ret = efi_dp_from_name(argv[3], argv[4], argv[5],
> -					       &device_path, &file_path);
> +					       &device_path, &fp_free);

The semantics of efi_dp_from_name() seems odd as both "device_path" and
"file_path" (or "fp_free") may partially contain a duplicated device path.
That is why "device_path" is not used after this line.

Although this behavior has not changed since my initial implementation,
"file_path" should not have included preceding device path components
other than the file path media device path.

Anyhow, we can pass NULL instead of "&device_path" for now.

>  			if (ret != EFI_SUCCESS) {
>  				printf("Cannot create device path for \"%s %s\"\n",
>  				       argv[3], argv[4]);
>  				r = CMD_RET_FAILURE;
>  				goto out;
>  			}
> +			if (shortform)
> +				file_path = efi_dp_shorten(fp_free);
> +			if (!file_path)
> +				file_path = fp_free;
>  			fp_size += efi_dp_size(file_path) +
>  				sizeof(struct efi_device_path);
>  			argc -= 5;
>  			argv += 5;
> -		} else if (!strcmp(argv[0], "-i")) {
> +			break;
> +		case 'i':
> +			shortform = 1;
> +			/* fallthrough */
> +		case 'I':
>  			if (argc < 3 || initrd_dp) {
>  				r = CMD_RET_USAGE;
>  				goto out;
>  			}
>  
> -			initrd_dp = create_initrd_dp(argv[1], argv[2], argv[3]);
> +			initrd_dp = create_initrd_dp(argv[1], argv[2], argv[3],
> +						     shortform);
>  			if (!initrd_dp) {
>  				printf("Cannot add an initrd\n");
>  				r = CMD_RET_FAILURE;
> @@ -876,7 +901,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  			argv += 3;
>  			fp_size += efi_dp_size(initrd_dp) +
>  				sizeof(struct efi_device_path);
> -		} else if (!strcmp(argv[0], "-s")) {
> +			break;
> +		case 's':
>  			if (argc < 1 || lo.optional_data) {
>  				r = CMD_RET_USAGE;
>  				goto out;
> @@ -884,7 +910,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  			lo.optional_data = (const u8 *)argv[1];
>  			argc -= 1;
>  			argv += 1;
> -		} else {
> +			break;
> +		default:
>  			r = CMD_RET_USAGE;
>  			goto out;
>  		}
> @@ -927,7 +954,7 @@ out:
>  	efi_free_pool(final_fp);
>  	efi_free_pool(initrd_dp);
>  	efi_free_pool(device_path);
> -	efi_free_pool(file_path);
> +	efi_free_pool(fp_free);
>  	free(lo.label);
>  
>  	return r;
> @@ -1571,12 +1598,11 @@ static int do_efidebug(struct cmd_tbl *cmdtp, int flag,
>  static char efidebug_help_text[] =
>  	"  - UEFI Shell-like interface to configure UEFI environment\n"
>  	"\n"
> -	"efidebug boot add "
> -	"-b <bootid> <label> <interface> <devnum>[:<part>] <file path> "
> -	"-i <interface> <devnum>[:<part>] <initrd file path> "
> -	"-s '<optional data>'\n"
> -	"  - set UEFI BootXXXX variable\n"
> -	"    <load options> will be passed to UEFI application\n"
> +	"efidebug boot add - set UEFI BootXXXX variable\n"
> +	"  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
> +	"  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
> +	"  (-b, -i for short form device path)\n"

I know you want to use short-forms (-b/-i) as default, but this change of meanings
potentially breaks the backward compatibility of command syntax although it's not a bug fix.

-Takahiro Akashi


> +	"  -s '<optional data>'\n"
>  	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
>  	"  - delete UEFI BootXXXX variables\n"
>  	"efidebug boot dump\n"
> -- 
> 2.34.1
>