[PATCH 3/8 v2] efi_loader: Add size checks to efi_create_indexed_name()

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> Although the function description states the caller must provide a
> sufficient buffer, it's better to have in function checks and ensure
> the destination buffer can hold the intended variable name.
>
> So let's add an extra argument with the buffer size and check that
> before copying.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
>   include/efi_loader.h        |  3 ++-
>   lib/efi_loader/efi_string.c | 10 ++++++++--
>   test/unicode_ut.c           |  2 +-
>   3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3c68b85b68e9..af30dbafab77 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -810,7 +810,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   void efi_memcpy_runtime(void *dest, const void *src, size_t n);
>
>   /* commonly used helper function */
> -u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index);
> +u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
> +			     unsigned int index);
>
>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */

Please, rebase upon origin/next.

With this patch U-Boot does not compile:

lib/efi_loader/efi_capsule.c: In function ‘set_capsule_result’:
lib/efi_loader/efi_capsule.c:76:43: error: passing argument 2 of
‘efi_create_indexed_name’ makes integer from pointer without a cast
[-Werror=int-conversion]
    76 |  efi_create_indexed_name(variable_name16, "Capsule", index);
       |                                           ^~~~~~~~~
       |                                           |
       |                                           char *

You missed to update lib/efi_loader/efi_capsule.c as you series is based
on origin/master.

Best regards

Heinrich

>
> diff --git a/lib/efi_loader/efi_string.c b/lib/efi_loader/efi_string.c
> index 3de721f06c7f..962724228866 100644
> --- a/lib/efi_loader/efi_string.c
> +++ b/lib/efi_loader/efi_string.c
> @@ -23,13 +23,19 @@
>    * Return: A pointer to the next position after the created string
>    *	   in @buffer, or NULL otherwise
>    */
> -u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index)
> +u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
> +			     unsigned int index)
>   {
>   	u16 *p = buffer;
>   	char index_buf[5];
> +	size_t size;
>
> +	size = (utf8_utf16_strlen(name) * sizeof(u16) +
> +		sizeof(index_buf) * sizeof(u16));
> +	if (buffer_size < size)
> +		return NULL;
>   	utf8_utf16_strcpy(&p, name);
> -	sprintf(index_buf, "%04X", index);
> +	snprintf(index_buf, sizeof(index_buf), "%04X", index);
>   	utf8_utf16_strcpy(&p, index_buf);
>
>   	return p;
> diff --git a/test/unicode_ut.c b/test/unicode_ut.c
> index 33fc8b0ee1e2..6130ef0b5497 100644
> --- a/test/unicode_ut.c
> +++ b/test/unicode_ut.c
> @@ -603,7 +603,7 @@ static int unicode_test_efi_create_indexed_name(struct unit_test_state *uts)
>   	u16 *pos;
>
>   	memset(buf, 0xeb, sizeof(buf));
> -	pos = efi_create_indexed_name(buf, "Capsule", 0x0af9);
> +	pos = efi_create_indexed_name(buf, sizeof(buf), "Capsule", 0x0af9);
>
>   	ut_asserteq_mem(expected, buf, sizeof(expected));
>   	ut_asserteq(pos - buf, u16_strnlen(buf, SIZE_MAX));
>