From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heinrich Schuchardt Date: Wed, 30 Dec 2020 19:34:38 +0100 Subject: [PATCH 3/8 v2] efi_loader: Add size checks to efi_create_indexed_name() In-Reply-To: <20201230150722.154663-4-ilias.apalodimas@linaro.org> References: <20201230150722.154663-1-ilias.apalodimas@linaro.org> <20201230150722.154663-4-ilias.apalodimas@linaro.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > --- > 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)); >