Re: [PATCH v2 05/10] efi/libstub: Use ucs2_string library for UTF-16 to UTF-8 conversion

[Vincent Mailhol <mailhol@kernel.org>]

On 9/9/26 13:55, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Don't rely on sprintf() with a wide string conversion modifier to
> convert the command line from UTF-16 to UTF-8. Instead, use the
> existing ucs2 string library routine that does the same. Note that while
> UEFI claims support for UTF-16, in practice it ignores surrogate pairs
> entirely, and so the simplified UCS-2 character set (where each
> character takes up exactly 2 bytes) is sufficient here.
> 
> This removes the only user of sprintf() in the EFI stub, so drop that
> function as well.
> 
> Since boot memory is plentiful on UEFI systems, just establish a worst
> case upper bound for the size of the buffer (which can never exceed
> COMMAND_LINE_SIZE), and allocate that first. Then, perform the
> conversion, and only fall back to processing the command line character
> by character if that resulted in truncation. This makes the common
> execution path much simpler.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/Makefile          |   3 +-
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 100 ++++++++------------
>  drivers/firmware/efi/libstub/vsprintf.c        |  11 ---
>  3 files changed, 41 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 77a2b2d74f3f..12c0c7deb5cb 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -66,7 +66,8 @@ KBUILD_AFLAGS			:= $(KBUILD_CFLAGS) -D__ASSEMBLY__
>  lib-y				:= efi-stub-helper.o gop.o secureboot.o tpm.o \
>  				   file.o mem.o random.o randomalloc.o pci.o \
>  				   skip_spaces.o lib-cmdline.o lib-ctype.o \
> -				   alignedmem.o printk.o vsprintf.o
> +				   alignedmem.o printk.o vsprintf.o \
> +				   lib-ucs2_string.o
>  
>  # include the stub's libfdt dependencies from lib/ when needed
>  libfdt-deps			:= fdt_rw.c fdt_ro.c fdt_wip.c fdt.c \
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index f27f2e1f0019..4b51a0bf0e66 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -12,6 +12,7 @@
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
>  #include <linux/overflow.h>
> +#include <linux/ucs2_string.h>
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>  
> @@ -334,81 +335,58 @@ char *efi_convert_cmdline(efi_loaded_image_t *image)
>  {
>  	const efi_char16_t *options = efi_table_attr(image, load_options);
>  	u32 options_size = efi_table_attr(image, load_options_size);
> -	int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
> -	unsigned long cmdline_addr = 0;
> -	const efi_char16_t *s2;
> -	bool in_quote = false;
> +	unsigned long options_chars = 0;
> +	unsigned long cmdline_bytes;
>  	efi_status_t status;
> -	u32 options_chars;
> +	char *cmdline_addr;
>  
>  	if (options_size > 0)
>  		efi_measure_tagged_event((unsigned long)options, options_size,
>  					 EFISTUB_EVT_LOAD_OPTIONS);
>  
>  	efi_apply_loadoptions_quirk((const void **)&options, &options_size);
> -	options_chars = options_size / sizeof(efi_char16_t);
> -
> -	if (options) {
> -		s2 = options;
> -		while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
> -			efi_char16_t c = *s2++;
> -
> -			if (c < 0x80) {
> -				if (c == L'\0' || c == L'\n')
                                                  ^^^^^^^^^^
This newline check is not present in the new version. The new code only
checks for NUL termination through ucs2_strnlen(). The patch description
says nothing about this, so maybe this is an omission?

> -					break;
> -				if (c == L'"')
> -					in_quote = !in_quote;
> -				else if (!in_quote && isspace((char)c))
> -					safe_options_bytes = options_bytes;
> -
> -				options_bytes++;
> -				continue;
> -			}
> -
> -			/*
> -			 * Get the number of UTF-8 bytes corresponding to a
> -			 * UTF-16 character.
> -			 * The first part handles everything in the BMP.
> -			 */
> -			options_bytes += 2 + (c >= 0x800);
> -			/*
> -			 * Add one more byte for valid surrogate pairs. Invalid
> -			 * surrogates will be replaced with 0xfffd and take up
> -			 * only 3 bytes.
> -			 */
> -			if ((c & 0xfc00) == 0xd800) {
> -				/*
> -				 * If the very last word is a high surrogate,
> -				 * we must ignore it since we can't access the
> -				 * low surrogate.
> -				 */
> -				if (!options_chars) {
> -					options_bytes -= 3;
> -				} else if ((*s2 & 0xfc00) == 0xdc00) {
> -					options_bytes++;
> -					options_chars--;
> -					s2++;
> -				}
> -			}
> -		}
> -		if (options_bytes >= COMMAND_LINE_SIZE) {
> -			options_bytes = safe_options_bytes;
> -			efi_err("Command line is too long: truncated to %d bytes\n",
> -				options_bytes);
> -		}
> -	}
> +	if (options)
> +		options_chars = ucs2_strnlen(options,
> +					     options_size / sizeof(efi_char16_t));
>  
> -	options_bytes++;	/* NUL termination */
> +	/* Each UCS-2 char takes up at most 3 UTF-8 bytes */
> +	cmdline_bytes = min(3 * options_chars, COMMAND_LINE_SIZE - 1) + 1;
>  
> -	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes,
> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, cmdline_bytes,
>  			     (void **)&cmdline_addr);
>  	if (status != EFI_SUCCESS)
>  		return NULL;
>  
> -	snprintf((char *)cmdline_addr, options_bytes, "%.*ls",
> -		 options_bytes - 1, options);
> +	if (ucs2_as_utf8_l(cmdline_addr, options, options_chars,
> +			   cmdline_bytes) >= COMMAND_LINE_SIZE) {
> +		/*
> +		 * The output fills up the entire buffer, and may have been
> +		 * truncated. This can only happen when options_bytes equals
> +		 * COMMAND_LINE_SIZE.

If truncation occurs because there is not enough space left for a
multi-byte character, ucs2_as_utf8_l() returns a value below
COMMAND_LINE_SIZE.

Should the test instead check whether all the options_chars were consumed?

> +		 * Work backwards through the buffer to find a safe truncation
> +		 * point (i.e., a blank character not inside a quoted string).
> +		 */
> +		int safe_pos[2] = {};
> +		int in_quote = 0;
> +
> +		for (int i = COMMAND_LINE_SIZE - 1; i >= 0; i--) {
> +			char c = cmdline_addr[i];
> +
> +			if (!c)
> +				return cmdline_addr;
> +			else if (c == '"')
> +				in_quote ^= 1;
> +			else if (!safe_pos[in_quote] && isspace(c))
> +				safe_pos[in_quote] = i;
> +		}
> +
> +		efi_err("Command line is too long: truncated to %d bytes\n",
> +			safe_pos[in_quote]);
> +		cmdline_addr[safe_pos[in_quote]] = '\0';
> +	}
>  
> -	return (char *)cmdline_addr;
> +	return cmdline_addr;
>  }
>  
>  /**
> diff --git a/drivers/firmware/efi/libstub/vsprintf.c b/drivers/firmware/efi/libstub/vsprintf.c
> index 71c71c222346..dba136679172 100644
> --- a/drivers/firmware/efi/libstub/vsprintf.c
> +++ b/drivers/firmware/efi/libstub/vsprintf.c
> @@ -551,14 +551,3 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list ap)
>  
>  	return pos;
>  }
> -
> -int snprintf(char *buf, size_t size, const char *fmt, ...)
> -{
> -	va_list args;
> -	int i;
> -
> -	va_start(args, fmt);
> -	i = vsnprintf(buf, size, fmt, args);
> -	va_end(args);
> -	return i;> -}


Yours sincerely,
Vincent Mailhol