load_elf: fix iterator type in glue

[PATCH] load_elf: fix iterator type in glue

Posted by Anastasia Belova 11 months, 1 week ago

file_size is uint32_t, so j < file_size should be
uint32_t too.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 0a5c258fe6..1defccaa71 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;
+                uint32_t j;
                 for (j = 0; j < file_size; j += (1 << data_swab)) {
                     uint8_t *dp = data + j;
                     switch (data_swab) {
-- 
2.30.2

Re: [PATCH] load_elf: fix iterator type in glue

Posted by Philippe Mathieu-Daudé 11 months ago

Hi,

On 21/12/23 09:08, Anastasia Belova wrote:
> file_size is uint32_t, so j < file_size should be
> uint32_t too.

file_size is of elf_word type, which is either uint32_t
or uint64_t.

Your explanation is not very clear... Maybe you want an unsigned type?
In that case, does the following makes your sanitizer happier?

-- >8 --
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 0a5c258fe6..03eba78c6e 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -502,4 +502,3 @@ static ssize_t glue(load_elf, SZ)(const char *name, 
int fd,
              if (data_swab) {
-                int j;
-                for (j = 0; j < file_size; j += (1 << data_swab)) {
+                for (unsigned j = 0; j < file_size; j += (1 << 
data_swab)) {
                      uint8_t *dp = data + j;
---

> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 0a5c258fe6..1defccaa71 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;
> +                uint32_t j;
>                   for (j = 0; j < file_size; j += (1 << data_swab)) {
>                       uint8_t *dp = data + j;
>                       switch (data_swab) {

Re: [PATCH] load_elf: fix iterator type in glue

Posted by Peter Maydell 10 months, 3 weeks ago

On Tue, 26 Dec 2023 at 12:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi,
>
> On 21/12/23 09:08, Anastasia Belova wrote:
> > file_size is uint32_t, so j < file_size should be
> > uint32_t too.
>
> file_size is of elf_word type, which is either uint32_t
> or uint64_t.
>
> Your explanation is not very clear... Maybe you want an unsigned type?
> In that case, does the following makes your sanitizer happier?

Since file_size is type 'elf_word', the iterator 'j' should
be the matching type. In practice nobody is loading 2GB ELF
files, so we don't really run into this, but it is a bug.

I agree with Philippe that it would be helpful if the
commit message:
 * is clear about the problem it is fixing
 * describes whether there are any real-world consequences
   of the issue and how severe (or not) they are

thanks
-- PMM

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

Posted by Anastasia Belova 10 months, 2 weeks 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 10 months, 2 weeks 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 10 months, 2 weeks 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 10 months, 2 weeks 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