| Message ID | 20201230150722.154663-4-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Change logic of EFI LoadFile2 protocol for initrd loading | expand |
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@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)); >
On Wed, Dec 30, 2020 at 07:34:38PM +0100, Heinrich Schuchardt wrote: > 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@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. Bah sorry! I'll rebase this on top of master and send it as a seperate patchset. Cheers /Ilias
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) */ 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));
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@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(-) -- 2.30.0