load_elf: fix iterators' types for elf file processing

[PATCH v2] load_elf: fix iterators' types for elf file processing

Posted by Anastasia Belova 8 months, 1 week ago

i and size should be the same type as ehdr.e_phnum (Elf32_Half or
Elf64_Half) to avoid overflows. So the bigger one is chosen.

j should be the same type as file_size for the same reasons.

This commit fixes a minor bug, maybe even a typo.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 7ef295ea5b ("loader: Add data swap option to load-elf")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
 include/hw/elf_ops.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 0a5c258fe6..6e807708f3 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -325,7 +325,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
 {
     struct elfhdr ehdr;
     struct elf_phdr *phdr = NULL, *ph;
-    int size, i;
+    Elf64_Half size, i;
     ssize_t total_size;
     elf_word mem_size, file_size, data_offset;
     uint64_t addr, low = (uint64_t)-1, high = 0;
@@ -464,7 +464,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
                  * the ROM overlap check in loader.c, so we don't try to
                  * explicitly detect those here.
                  */
-                int j;
+                Elf64_Half j;
                 elf_word zero_start = ph->p_paddr + file_size;
                 elf_word zero_end = ph->p_paddr + mem_size;
 
@@ -500,7 +500,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
             }
 
             if (data_swab) {
-                int j;
+                elf_word j;
                 for (j = 0; j < file_size; j += (1 << data_swab)) {
                     uint8_t *dp = data + j;
                     switch (data_swab) {
-- 
2.30.2

Re: [PATCH v2] load_elf: fix iterators' types for elf file processing

Posted by Peter Maydell 8 months, 1 week ago

On Fri, 12 Jan 2024 at 11:46, Anastasia Belova <abelova@astralinux.ru> wrote:
>
> i and size should be the same type as ehdr.e_phnum (Elf32_Half or
> Elf64_Half) to avoid overflows. So the bigger one is chosen.
>
> j should be the same type as file_size for the same reasons.
>
> This commit fixes a minor bug, maybe even a typo.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 7ef295ea5b ("loader: Add data swap option to load-elf")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
>  include/hw/elf_ops.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 0a5c258fe6..6e807708f3 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -325,7 +325,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
>  {
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
> -    int size, i;
> +    Elf64_Half size, i;

Elf64_Half is a 16 bit type -- this doesn't seem right.

In particular we use 'size' to store "ehdr.e_phnum * sizeof(phdr[0])"
so even if we know e_phnum fits in a 16 bit type the
multiplication is not guaranteed to; so this change actually
introduces a bug. The type we need for what we're doing needs
to be big enough to store the result of that multiplication,
so anything bigger than 16 bits will be fine, including 'int'
(since we know the RHS is a small number). We could change
this to 'size_t' or 'ssize_t', I suppose, but it doesn't
really seem necessary.

(Side note: whatever your static analyser is could presumbaly be
doing better about identifying overflows here -- it ought to be
able to figure out that "unsigned 16 bit value * constant 64"
is not going to overflow a signed 32 bit type.)

i is only used as the iterator through the program headers,
so it wouldn't be wrong to make it a 16 bit type, but there's
really no need to constrain it -- an 'int' is fine.

Also, these functions have macro magic so we create both the
32-bit and 64-bit elf versions from one bit of source, so it
is not in general right for them to have a type in them that
is specific to one or the other, as Elf64_Half is.

>      ssize_t total_size;
>      elf_word mem_size, file_size, data_offset;
>      uint64_t addr, low = (uint64_t)-1, high = 0;
> @@ -464,7 +464,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
>                   * the ROM overlap check in loader.c, so we don't try to
>                   * explicitly detect those here.
>                   */
> -                int j;
> +                Elf64_Half j;

This case is like 'i' above -- it's not a bug to use a 16 bit
type for the iterator variable, but it isn't necessary,
and we shouldn't use an Elf64* type.

>                  elf_word zero_start = ph->p_paddr + file_size;
>                  elf_word zero_end = ph->p_paddr + mem_size;
>
> @@ -500,7 +500,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
>              }
>
>              if (data_swab) {
> -                int j;
> +                elf_word j;
>                  for (j = 0; j < file_size; j += (1 << data_swab)) {
>                      uint8_t *dp = data + j;
>                      switch (data_swab) {

This change is OK and necessary. It fixes a minor bug:
if we are loading an ELF file that contains a segment
whose data in the ELF file is larger than 2GB and we need
to byteswap that data, we would previously get this wrong.
(We should mention this in the commit message.)

thanks
-- PMM

[PATCH v3] load_elf: fix iterator's type for elf file processing

Posted by Anastasia Belova 8 months, 1 week ago

j is used while loading an ELF file to byteswap segments'
data. If data is larger than 2GB an overflow may happen.
So j should be elf_word.

This commit fixes a minor bug, maybe even a typo.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 7ef295ea5b ("loader: Add data swap option to load-elf")
Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
---
v2: fix type of j
v3: remove changes for i, size and another j
Thanks for your patience.
 include/hw/elf_ops.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 0a5c258fe6..9c35d1b9da 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -500,7 +500,7 @@ static ssize_t glue(load_elf, SZ)(const char *name, int fd,
             }
 
             if (data_swab) {
-                int j;
+                elf_word j;
                 for (j = 0; j < file_size; j += (1 << data_swab)) {
                     uint8_t *dp = data + j;
                     switch (data_swab) {
-- 
2.30.2

Re: [PATCH v3] load_elf: fix iterator's type for elf file processing

Posted by Peter Maydell 8 months, 1 week ago

On Mon, 15 Jan 2024 at 09:22, Anastasia Belova <abelova@astralinux.ru> wrote:
>
> j is used while loading an ELF file to byteswap segments'
> data. If data is larger than 2GB an overflow may happen.
> So j should be elf_word.
>
> This commit fixes a minor bug, maybe even a typo.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 7ef295ea5b ("loader: Add data swap option to load-elf")
> Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> ---
> v2: fix type of j
> v3: remove changes for i, size and another j
> Thanks for your patience.
>  include/hw/elf_ops.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to target-arm.next, with a minor tweak to the commit
message. Thanks for your contribution!

-- PMM