[PATCH 0/4] libelf: misc adjustments

[PATCH 0/4] libelf: misc adjustments

[Jan Beulich]

1: section index 0 is special
2: use UINT_MAX
3: type adjustments
4: treat phdr and shdr similarly

Signed-off-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/4] libelf: section index 0 is special

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

When iterating over sections, table entry zero needs to be ignored.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -536,7 +536,7 @@ elf_errorstatus elf_xen_parse(struct elf
     if ( xen_elfnotes == 0 )
     {
         count = elf_shdr_count(elf);
-        for ( i = 0; i < count; i++ )
+        for ( i = 1; i < count; i++ )
         {
             shdr = elf_shdr_by_index(elf, i);
             if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -79,7 +79,7 @@ elf_errorstatus elf_init(struct elf_bina
 
     /* Find symbol table and symbol string table. */
     count = elf_shdr_count(elf);
-    for ( i = 0; i < count; i++ )
+    for ( i = 1; i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_na
     const char *sname;
     unsigned i;
 
-    for ( i = 0; i < count; i++ )
+    for ( i = 1; i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )




[-- Attachment #2: libelf-no-section-0.patch --]
[-- Type: text/plain, Size: 1393 bytes --]

libelf: section index 0 is special

When iterating over sections, table entry zero needs to be ignored.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -536,7 +536,7 @@ elf_errorstatus elf_xen_parse(struct elf
     if ( xen_elfnotes == 0 )
     {
         count = elf_shdr_count(elf);
-        for ( i = 0; i < count; i++ )
+        for ( i = 1; i < count; i++ )
         {
             shdr = elf_shdr_by_index(elf, i);
             if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -79,7 +79,7 @@ elf_errorstatus elf_init(struct elf_bina
 
     /* Find symbol table and symbol string table. */
     count = elf_shdr_count(elf);
-    for ( i = 0; i < count; i++ )
+    for ( i = 1; i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_na
     const char *sname;
     unsigned i;
 
-    for ( i = 0; i < count; i++ )
+    for ( i = 1; i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/4] libelf: use UINT_MAX

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

While Xen indeed doesn't have limits.h, it still does have UINT_MAX, so
we should avoid open coding it (and perhaps - even if unlikely -
getting it wrong).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -43,6 +43,7 @@
 #include <string.h>
 #include <stddef.h>
 #include <inttypes.h>
+#include <limits.h>
 #ifdef __sun__
 #include <sys/byteorder.h>
 #define bswap_16(x) BSWAP_16(x)
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -131,9 +131,10 @@ unsigned elf_shdr_count(struct elf_binar
 {
     unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
     uint64_t max = elf->size / sizeof(Elf32_Shdr);
-    if (max > ~(unsigned)0)
-        max = ~(unsigned)0; /* Xen doesn't have limits.h :-/ */
-    if (count > max)
+
+    if ( max > UINT_MAX )
+        max = UINT_MAX;
+    if ( count > max )
     {
         elf_mark_broken(elf, "far too many section headers");
         count = max;




[-- Attachment #2: libelf-use-UINT_MAX.patch --]
[-- Type: text/plain, Size: 1062 bytes --]

libelf: use UINT_MAX

While Xen indeed doesn't have limits.h, it still does have UINT_MAX, so
we should avoid open coding it (and perhaps - even if unlikely -
getting it wrong).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-private.h
+++ b/xen/common/libelf/libelf-private.h
@@ -43,6 +43,7 @@
 #include <string.h>
 #include <stddef.h>
 #include <inttypes.h>
+#include <limits.h>
 #ifdef __sun__
 #include <sys/byteorder.h>
 #define bswap_16(x) BSWAP_16(x)
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -131,9 +131,10 @@ unsigned elf_shdr_count(struct elf_binar
 {
     unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
     uint64_t max = elf->size / sizeof(Elf32_Shdr);
-    if (max > ~(unsigned)0)
-        max = ~(unsigned)0; /* Xen doesn't have limits.h :-/ */
-    if (count > max)
+
+    if ( max > UINT_MAX )
+        max = UINT_MAX;
+    if ( count > max )
     {
         elf_mark_broken(elf, "far too many section headers");
         count = max;

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/4] libelf: type adjustments

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3632 bytes --]

Don't needlessly use uint64_t when unsigned suffices.

Also don't open code elf_phdr_count() and replace a redundant call to
elf_shdr_count().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -36,7 +36,8 @@ struct elf_sym_header {
 elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t size)
 {
     ELF_HANDLE_DECL(elf_shdr) shdr;
-    uint64_t i, count, section, offset, link;
+    unsigned i, count, section, link;
+    uint64_t offset;
 
     if ( !elf_is_elfbinary(image_input, size) )
     {
@@ -89,7 +90,7 @@ elf_errorstatus elf_init(struct elf_bina
             continue;
 
         link = elf_uval(elf, shdr, sh_link);
-        if ( link == SHN_UNDEF || link >= elf_shdr_count(elf) )
+        if ( link == SHN_UNDEF || link >= count )
             /* out-of-bounds link value. */
             break;
 
@@ -443,11 +444,10 @@ do {
 void elf_parse_binary(struct elf_binary *elf)
 {
     ELF_HANDLE_DECL(elf_phdr) phdr;
-    uint64_t low = -1;
-    uint64_t high = 0;
-    uint64_t i, count, paddr, memsz;
+    uint64_t low = -1, high = 0, paddr, memsz;
+    unsigned i, count;
 
-    count = elf_uval(elf, elf->ehdr, e_phnum);
+    count = elf_phdr_count(elf);
     for ( i = 0; i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
@@ -474,7 +474,8 @@ void elf_parse_binary(struct elf_binary
 elf_errorstatus elf_load_binary(struct elf_binary *elf)
 {
     ELF_HANDLE_DECL(elf_phdr) phdr;
-    uint64_t i, count, paddr, offset, filesz, memsz;
+    uint64_t paddr, offset, filesz, memsz;
+    unsigned i, count;
     elf_ptrval dest;
     /*
      * Let bizarre ELFs write the output image up to twice; this
@@ -483,7 +484,7 @@ elf_errorstatus elf_load_binary(struct e
      */
     uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
 
-    count = elf_uval(elf, elf->ehdr, e_phnum);
+    count = elf_phdr_count(elf);
     for ( i = 0; i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
@@ -512,7 +513,7 @@ elf_errorstatus elf_load_binary(struct e
         remain_allow_copy -= memsz;
 
         elf_msg(elf,
-                "ELF: phdr %" PRIu64 " at %#"ELF_PRPTRVAL" -> %#"ELF_PRPTRVAL"\n",
+                "ELF: phdr %u at %#"ELF_PRPTRVAL" -> %#"ELF_PRPTRVAL"\n",
                 i, dest, (elf_ptrval)(dest + filesz));
         if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz, memsz) != 0 )
             return -1;
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -149,10 +149,9 @@ unsigned elf_phdr_count(struct elf_binar
 
 ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name)
 {
-    uint64_t count = elf_shdr_count(elf);
+    unsigned i, count = elf_shdr_count(elf);
     ELF_HANDLE_DECL(elf_shdr) shdr;
     const char *sname;
-    unsigned i;
 
     for ( i = 1; i < count; i++ )
     {
@@ -169,7 +168,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_na
 
 ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index)
 {
-    uint64_t count = elf_shdr_count(elf);
+    unsigned count = elf_shdr_count(elf);
     elf_ptrval ptr;
 
     if ( index >= count )
@@ -183,7 +182,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_in
 
 ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index)
 {
-    uint64_t count = elf_uval(elf, elf->ehdr, e_phnum);
+    unsigned count = elf_phdr_count(elf);
     elf_ptrval ptr;
 
     if ( index >= count )




[-- Attachment #2: libelf-types.patch --]
[-- Type: text/plain, Size: 3654 bytes --]

libelf: type adjustments

Don't needlessly use uint64_t when unsigned suffices.

Also don't open code elf_phdr_count() and replace a redundant call to
elf_shdr_count().

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -36,7 +36,8 @@ struct elf_sym_header {
 elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t size)
 {
     ELF_HANDLE_DECL(elf_shdr) shdr;
-    uint64_t i, count, section, offset, link;
+    unsigned i, count, section, link;
+    uint64_t offset;
 
     if ( !elf_is_elfbinary(image_input, size) )
     {
@@ -89,7 +90,7 @@ elf_errorstatus elf_init(struct elf_bina
             continue;
 
         link = elf_uval(elf, shdr, sh_link);
-        if ( link == SHN_UNDEF || link >= elf_shdr_count(elf) )
+        if ( link == SHN_UNDEF || link >= count )
             /* out-of-bounds link value. */
             break;
 
@@ -443,11 +444,10 @@ do {
 void elf_parse_binary(struct elf_binary *elf)
 {
     ELF_HANDLE_DECL(elf_phdr) phdr;
-    uint64_t low = -1;
-    uint64_t high = 0;
-    uint64_t i, count, paddr, memsz;
+    uint64_t low = -1, high = 0, paddr, memsz;
+    unsigned i, count;
 
-    count = elf_uval(elf, elf->ehdr, e_phnum);
+    count = elf_phdr_count(elf);
     for ( i = 0; i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
@@ -474,7 +474,8 @@ void elf_parse_binary(struct elf_binary
 elf_errorstatus elf_load_binary(struct elf_binary *elf)
 {
     ELF_HANDLE_DECL(elf_phdr) phdr;
-    uint64_t i, count, paddr, offset, filesz, memsz;
+    uint64_t paddr, offset, filesz, memsz;
+    unsigned i, count;
     elf_ptrval dest;
     /*
      * Let bizarre ELFs write the output image up to twice; this
@@ -483,7 +484,7 @@ elf_errorstatus elf_load_binary(struct e
      */
     uint64_t remain_allow_copy = (uint64_t)elf->dest_size * 2;
 
-    count = elf_uval(elf, elf->ehdr, e_phnum);
+    count = elf_phdr_count(elf);
     for ( i = 0; i < count; i++ )
     {
         phdr = elf_phdr_by_index(elf, i);
@@ -512,7 +513,7 @@ elf_errorstatus elf_load_binary(struct e
         remain_allow_copy -= memsz;
 
         elf_msg(elf,
-                "ELF: phdr %" PRIu64 " at %#"ELF_PRPTRVAL" -> %#"ELF_PRPTRVAL"\n",
+                "ELF: phdr %u at %#"ELF_PRPTRVAL" -> %#"ELF_PRPTRVAL"\n",
                 i, dest, (elf_ptrval)(dest + filesz));
         if ( elf_load_image(elf, dest, ELF_IMAGE_BASE(elf) + offset, filesz, memsz) != 0 )
             return -1;
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -149,10 +149,9 @@ unsigned elf_phdr_count(struct elf_binar
 
 ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name)
 {
-    uint64_t count = elf_shdr_count(elf);
+    unsigned i, count = elf_shdr_count(elf);
     ELF_HANDLE_DECL(elf_shdr) shdr;
     const char *sname;
-    unsigned i;
 
     for ( i = 1; i < count; i++ )
     {
@@ -169,7 +168,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_na
 
 ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_index(struct elf_binary *elf, unsigned index)
 {
-    uint64_t count = elf_shdr_count(elf);
+    unsigned count = elf_shdr_count(elf);
     elf_ptrval ptr;
 
     if ( index >= count )
@@ -183,7 +182,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_in
 
 ELF_HANDLE_DECL(elf_phdr) elf_phdr_by_index(struct elf_binary *elf, unsigned index)
 {
-    uint64_t count = elf_uval(elf, elf->ehdr, e_phnum);
+    unsigned count = elf_phdr_count(elf);
     elf_ptrval ptr;
 
     if ( index >= count )

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 4/4] libelf: treat phdr and shdr similarly

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4242 bytes --]

Just like elf_shdr_count(), elf_phdr_count() better bounds checks the
value.

Add table entry size checks to elf_init().

Also both program and section headers are optional, and hence their
checking better is done conditionally only when any such headers are
present.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -52,24 +52,45 @@ elf_errorstatus elf_init(struct elf_bina
     elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
     elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);
 
-    /* Sanity check phdr. */
-    offset = elf_uval(elf, elf->ehdr, e_phoff) +
-        elf_uval(elf, elf->ehdr, e_phentsize) * elf_phdr_count(elf);
-    if ( offset > elf->size )
+    /* Sanity check phdr if present. */
+    count = elf_phdr_count(elf);
+    if ( count )
     {
-        elf_err(elf, "ELF: phdr overflow (off %" PRIx64 " > size %lx)\n",
-                offset, (unsigned long)elf->size);
-        return -1;
+        if ( elf_uval(elf, elf->ehdr, e_phentsize) <
+             elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
+        {
+            elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
+                    elf_uval(elf, elf->ehdr, e_phentsize));
+            return -1;
+        }
+        offset = elf_uval(elf, elf->ehdr, e_phoff) +
+            elf_uval(elf, elf->ehdr, e_phentsize) * count;
+        if ( offset > elf->size )
+        {
+            elf_err(elf, "ELF: phdr overflow (off %" PRIx64 " > size %lx)\n",
+                    offset, (unsigned long)elf->size);
+            return -1;
+        }
     }
 
-    /* Sanity check shdr. */
-    offset = elf_uval(elf, elf->ehdr, e_shoff) +
-        elf_uval(elf, elf->ehdr, e_shentsize) * elf_shdr_count(elf);
-    if ( offset > elf->size )
+    /* Sanity check shdr if present. */
+    count = elf_shdr_count(elf);
+    if ( count )
     {
-        elf_err(elf, "ELF: shdr overflow (off %" PRIx64 " > size %lx)\n",
-                offset, (unsigned long)elf->size);
-        return -1;
+        if ( elf_uval(elf, elf->ehdr, e_shentsize) < elf_size(elf, shdr) )
+        {
+            elf_err(elf, "ELF: shdr too small (%" PRIu64 ")\n",
+                    elf_uval(elf, elf->ehdr, e_shentsize));
+            return -1;
+        }
+        offset = elf_uval(elf, elf->ehdr, e_shoff) +
+            elf_uval(elf, elf->ehdr, e_shentsize) * count;
+        if ( offset > elf->size )
+        {
+            elf_err(elf, "ELF: shdr overflow (off %" PRIx64 " > size %lx)\n",
+                    offset, (unsigned long)elf->size);
+            return -1;
+        }
     }
 
     /* Find section string table. */
@@ -79,7 +100,6 @@ elf_errorstatus elf_init(struct elf_bina
         elf->sec_strtab = elf_section_start(elf, shdr);
 
     /* Find symbol table and symbol string table. */
-    count = elf_shdr_count(elf);
     for ( i = 1; i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,8 +130,11 @@ uint64_t elf_round_up(struct elf_binary
 unsigned elf_shdr_count(struct elf_binary *elf)
 {
     unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
-    uint64_t max = elf->size / sizeof(Elf32_Shdr);
+    uint64_t max;
 
+    if ( !count )
+        return 0;
+    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
     if ( max > UINT_MAX )
         max = UINT_MAX;
     if ( count > max )
@@ -144,7 +147,20 @@ unsigned elf_shdr_count(struct elf_binar
 
 unsigned elf_phdr_count(struct elf_binary *elf)
 {
-    return elf_uval(elf, elf->ehdr, e_phnum);
+    unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+    uint64_t max;
+
+    if ( !count )
+        return 0;
+    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+    if ( max > UINT_MAX )
+        max = UINT_MAX;
+    if ( count > max )
+    {
+        elf_mark_broken(elf, "far too many program headers");
+        count = max;
+    }
+    return count;
 }
 
 ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name)



[-- Attachment #2: libelf-phdr-shdr.patch --]
[-- Type: text/plain, Size: 4279 bytes --]

libelf: treat phdr and shdr similarly

Just like elf_shdr_count(), elf_phdr_count() better bounds checks the
value.

Add table entry size checks to elf_init().

Also both program and section headers are optional, and hence their
checking better is done conditionally only when any such headers are
present.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -52,24 +52,45 @@ elf_errorstatus elf_init(struct elf_bina
     elf->class = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_CLASS]);
     elf->data = elf_uval_3264(elf, elf->ehdr, e32.e_ident[EI_DATA]);
 
-    /* Sanity check phdr. */
-    offset = elf_uval(elf, elf->ehdr, e_phoff) +
-        elf_uval(elf, elf->ehdr, e_phentsize) * elf_phdr_count(elf);
-    if ( offset > elf->size )
+    /* Sanity check phdr if present. */
+    count = elf_phdr_count(elf);
+    if ( count )
     {
-        elf_err(elf, "ELF: phdr overflow (off %" PRIx64 " > size %lx)\n",
-                offset, (unsigned long)elf->size);
-        return -1;
+        if ( elf_uval(elf, elf->ehdr, e_phentsize) <
+             elf_size(elf, ELF_HANDLE_DECL(elf_phdr)) )
+        {
+            elf_err(elf, "ELF: phdr too small (%" PRIu64 ")\n",
+                    elf_uval(elf, elf->ehdr, e_phentsize));
+            return -1;
+        }
+        offset = elf_uval(elf, elf->ehdr, e_phoff) +
+            elf_uval(elf, elf->ehdr, e_phentsize) * count;
+        if ( offset > elf->size )
+        {
+            elf_err(elf, "ELF: phdr overflow (off %" PRIx64 " > size %lx)\n",
+                    offset, (unsigned long)elf->size);
+            return -1;
+        }
     }
 
-    /* Sanity check shdr. */
-    offset = elf_uval(elf, elf->ehdr, e_shoff) +
-        elf_uval(elf, elf->ehdr, e_shentsize) * elf_shdr_count(elf);
-    if ( offset > elf->size )
+    /* Sanity check shdr if present. */
+    count = elf_shdr_count(elf);
+    if ( count )
     {
-        elf_err(elf, "ELF: shdr overflow (off %" PRIx64 " > size %lx)\n",
-                offset, (unsigned long)elf->size);
-        return -1;
+        if ( elf_uval(elf, elf->ehdr, e_shentsize) < elf_size(elf, shdr) )
+        {
+            elf_err(elf, "ELF: shdr too small (%" PRIu64 ")\n",
+                    elf_uval(elf, elf->ehdr, e_shentsize));
+            return -1;
+        }
+        offset = elf_uval(elf, elf->ehdr, e_shoff) +
+            elf_uval(elf, elf->ehdr, e_shentsize) * count;
+        if ( offset > elf->size )
+        {
+            elf_err(elf, "ELF: shdr overflow (off %" PRIx64 " > size %lx)\n",
+                    offset, (unsigned long)elf->size);
+            return -1;
+        }
     }
 
     /* Find section string table. */
@@ -79,7 +100,6 @@ elf_errorstatus elf_init(struct elf_bina
         elf->sec_strtab = elf_section_start(elf, shdr);
 
     /* Find symbol table and symbol string table. */
-    count = elf_shdr_count(elf);
     for ( i = 1; i < count; i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
--- a/xen/common/libelf/libelf-tools.c
+++ b/xen/common/libelf/libelf-tools.c
@@ -130,8 +130,11 @@ uint64_t elf_round_up(struct elf_binary
 unsigned elf_shdr_count(struct elf_binary *elf)
 {
     unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
-    uint64_t max = elf->size / sizeof(Elf32_Shdr);
+    uint64_t max;
 
+    if ( !count )
+        return 0;
+    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
     if ( max > UINT_MAX )
         max = UINT_MAX;
     if ( count > max )
@@ -144,7 +147,20 @@ unsigned elf_shdr_count(struct elf_binar
 
 unsigned elf_phdr_count(struct elf_binary *elf)
 {
-    return elf_uval(elf, elf->ehdr, e_phnum);
+    unsigned count = elf_uval(elf, elf->ehdr, e_phnum);
+    uint64_t max;
+
+    if ( !count )
+        return 0;
+    max = elf->size / elf_uval(elf, elf->ehdr, e_phentsize);
+    if ( max > UINT_MAX )
+        max = UINT_MAX;
+    if ( count > max )
+    {
+        elf_mark_broken(elf, "far too many program headers");
+        count = max;
+    }
+    return count;
 }
 
 ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_name(struct elf_binary *elf, const char *name)

[-- Attachment #3: Type: text/plain, Size: 127 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] libelf: section index 0 is special

[Konrad Rzeszutek Wilk]

On Tue, Dec 06, 2016 at 04:51:39AM -0700, Jan Beulich wrote:
> When iterating over sections, table entry zero needs to be ignored.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -536,7 +536,7 @@ elf_errorstatus elf_xen_parse(struct elf
>      if ( xen_elfnotes == 0 )
>      {
>          count = elf_shdr_count(elf);
> -        for ( i = 0; i < count; i++ )
> +        for ( i = 1; i < count; i++ )
>          {
>              shdr = elf_shdr_by_index(elf, i);
>              if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -79,7 +79,7 @@ elf_errorstatus elf_init(struct elf_bina
>  
>      /* Find symbol table and symbol string table. */
>      count = elf_shdr_count(elf);
> -    for ( i = 0; i < count; i++ )
> +    for ( i = 1; i < count; i++ )
>      {
>          shdr = elf_shdr_by_index(elf, i);
>          if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_na
>      const char *sname;
>      unsigned i;
>  
> -    for ( i = 0; i < count; i++ )
> +    for ( i = 1; i < count; i++ )
>      {
>          shdr = elf_shdr_by_index(elf, i);
>          if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
> 
> 
> 

> libelf: section index 0 is special
> 
> When iterating over sections, table entry zero needs to be ignored.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/libelf/libelf-dominfo.c
> +++ b/xen/common/libelf/libelf-dominfo.c
> @@ -536,7 +536,7 @@ elf_errorstatus elf_xen_parse(struct elf
>      if ( xen_elfnotes == 0 )
>      {
>          count = elf_shdr_count(elf);
> -        for ( i = 0; i < count; i++ )
> +        for ( i = 1; i < count; i++ )
>          {
>              shdr = elf_shdr_by_index(elf, i);
>              if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -79,7 +79,7 @@ elf_errorstatus elf_init(struct elf_bina
>  
>      /* Find symbol table and symbol string table. */
>      count = elf_shdr_count(elf);
> -    for ( i = 0; i < count; i++ )
> +    for ( i = 1; i < count; i++ )
>      {
>          shdr = elf_shdr_by_index(elf, i);
>          if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -153,7 +153,7 @@ ELF_HANDLE_DECL(elf_shdr) elf_shdr_by_na
>      const char *sname;
>      unsigned i;
>  
> -    for ( i = 0; i < count; i++ )
> +    for ( i = 1; i < count; i++ )
>      {
>          shdr = elf_shdr_by_index(elf, i);
>          if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] libelf: use UINT_MAX

[Konrad Rzeszutek Wilk]

On Tue, Dec 06, 2016 at 04:52:12AM -0700, Jan Beulich wrote:
> While Xen indeed doesn't have limits.h, it still does have UINT_MAX, so
> we should avoid open coding it (and perhaps - even if unlikely -
> getting it wrong).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] libelf: type adjustments

[Konrad Rzeszutek Wilk]

On Tue, Dec 06, 2016 at 04:52:42AM -0700, Jan Beulich wrote:
> Don't needlessly use uint64_t when unsigned suffices.
> 
> Also don't open code elf_phdr_count() and replace a redundant call to
> elf_shdr_count().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] libelf: treat phdr and shdr similarly

[Konrad Rzeszutek Wilk]

. snip..
All that above makes sense (and please put Reviewed-by from me on it), but this:

>  unsigned elf_shdr_count(struct elf_binary *elf)
>  {
>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
> -    uint64_t max = elf->size / sizeof(Elf32_Shdr);
> +    uint64_t max;
>  
> +    if ( !count )
> +        return 0;
> +    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);

Seems incorrect. The elf->size is the size of the image - including
the ELF and the data it contains.

But I presume the check is rather to make sure that if there
is no data, just an ELF sections  - that we don't roll over it.

In which case perhaps adding a comment saying:

/* If file has nothing but ELF this will catch us from rolling over the end.*/

or such?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] libelf: treat phdr and shdr similarly

[Jan Beulich]

>>> On 06.12.16 at 14:27, <konrad.wilk@oracle.com> wrote:
>>  unsigned elf_shdr_count(struct elf_binary *elf)
>>  {
>>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>> -    uint64_t max = elf->size / sizeof(Elf32_Shdr);
>> +    uint64_t max;
>>  
>> +    if ( !count )
>> +        return 0;
>> +    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
> 
> Seems incorrect. The elf->size is the size of the image - including
> the ELF and the data it contains.
> 
> But I presume the check is rather to make sure that if there
> is no data, just an ELF sections  - that we don't roll over it.
> 
> In which case perhaps adding a comment saying:
> 
> /* If file has nothing but ELF this will catch us from rolling over the end.*/
> 
> or such?

Well - there was no such comment before on the equivalent (but
slightly wrong) check which is being replaced here. If you really
think such a comment should be added now, I can certainly (albeit
a little reluctantly) do so - let me know.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] libelf: use UINT_MAX

[Roger Pau Monné]

On Tue, Dec 06, 2016 at 04:52:12AM -0700, Jan Beulich wrote:
> While Xen indeed doesn't have limits.h, it still does have UINT_MAX, so
> we should avoid open coding it (and perhaps - even if unlikely -
> getting it wrong).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/common/libelf/libelf-private.h
> +++ b/xen/common/libelf/libelf-private.h
> @@ -43,6 +43,7 @@
>  #include <string.h>
>  #include <stddef.h>
>  #include <inttypes.h>
> +#include <limits.h>
>  #ifdef __sun__
>  #include <sys/byteorder.h>
>  #define bswap_16(x) BSWAP_16(x)
> --- a/xen/common/libelf/libelf-tools.c
> +++ b/xen/common/libelf/libelf-tools.c
> @@ -131,9 +131,10 @@ unsigned elf_shdr_count(struct elf_binar
>  {
>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>      uint64_t max = elf->size / sizeof(Elf32_Shdr);
> -    if (max > ~(unsigned)0)
> -        max = ~(unsigned)0; /* Xen doesn't have limits.h :-/ */
> -    if (count > max)
> +
> +    if ( max > UINT_MAX )
> +        max = UINT_MAX;

Can't you use a min helper here? It would be clear IMHO.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/4] libelf: section index 0 is special

[Roger Pau Monné]

On Tue, Dec 06, 2016 at 04:51:39AM -0700, Jan Beulich wrote:
> When iterating over sections, table entry zero needs to be ignored.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] libelf: treat phdr and shdr similarly

[Konrad Rzeszutek Wilk]

On Tue, Dec 06, 2016 at 06:37:30AM -0700, Jan Beulich wrote:
> >>> On 06.12.16 at 14:27, <konrad.wilk@oracle.com> wrote:
> >>  unsigned elf_shdr_count(struct elf_binary *elf)
> >>  {
> >>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
> >> -    uint64_t max = elf->size / sizeof(Elf32_Shdr);
> >> +    uint64_t max;
> >>  
> >> +    if ( !count )
> >> +        return 0;
> >> +    max = elf->size / elf_uval(elf, elf->ehdr, e_shentsize);
> > 
> > Seems incorrect. The elf->size is the size of the image - including
> > the ELF and the data it contains.
> > 
> > But I presume the check is rather to make sure that if there
> > is no data, just an ELF sections  - that we don't roll over it.
> > 
> > In which case perhaps adding a comment saying:
> > 
> > /* If file has nothing but ELF this will catch us from rolling over the end.*/
> > 
> > or such?
> 
> Well - there was no such comment before on the equivalent (but
> slightly wrong) check which is being replaced here. If you really
> think such a comment should be added now, I can certainly (albeit
> a little reluctantly) do so - let me know.

It is up to you. I was thinking more in terms of somebody reading this
code and not realizing what we are checking for.

But maybe it is just me not having yet my coffee and other folks
looking at this would spot this right away.

> 
> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/4] libelf: type adjustments

[Roger Pau Monné]

On Tue, Dec 06, 2016 at 04:52:42AM -0700, Jan Beulich wrote:
> Don't needlessly use uint64_t when unsigned suffices.
> 
> Also don't open code elf_phdr_count() and replace a redundant call to
> elf_shdr_count().
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] libelf: use UINT_MAX

[Jan Beulich]

>>> On 06.12.16 at 14:49, <roger.pau@citrix.com> wrote:
>> --- a/xen/common/libelf/libelf-tools.c
>> +++ b/xen/common/libelf/libelf-tools.c
>> @@ -131,9 +131,10 @@ unsigned elf_shdr_count(struct elf_binar
>>  {
>>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
>>      uint64_t max = elf->size / sizeof(Elf32_Shdr);
>> -    if (max > ~(unsigned)0)
>> -        max = ~(unsigned)0; /* Xen doesn't have limits.h :-/ */
>> -    if (count > max)
>> +
>> +    if ( max > UINT_MAX )
>> +        max = UINT_MAX;
> 
> Can't you use a min helper here? It would be clear IMHO.

I'm afraid there's no min() available that would work (build) equally
in both libxc and hypervisor context.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 4/4] libelf: treat phdr and shdr similarly

[Roger Pau Monné]

On Tue, Dec 06, 2016 at 04:53:12AM -0700, Jan Beulich wrote:
> Just like elf_shdr_count(), elf_phdr_count() better bounds checks the
> value.
> 
> Add table entry size checks to elf_init().
> 
> Also both program and section headers are optional, and hence their
> checking better is done conditionally only when any such headers are
> present.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/4] libelf: use UINT_MAX

[Roger Pau Monné]

On Tue, Dec 06, 2016 at 07:09:12AM -0700, Jan Beulich wrote:
> >>> On 06.12.16 at 14:49, <roger.pau@citrix.com> wrote:
> >> --- a/xen/common/libelf/libelf-tools.c
> >> +++ b/xen/common/libelf/libelf-tools.c
> >> @@ -131,9 +131,10 @@ unsigned elf_shdr_count(struct elf_binar
> >>  {
> >>      unsigned count = elf_uval(elf, elf->ehdr, e_shnum);
> >>      uint64_t max = elf->size / sizeof(Elf32_Shdr);
> >> -    if (max > ~(unsigned)0)
> >> -        max = ~(unsigned)0; /* Xen doesn't have limits.h :-/ */
> >> -    if (count > max)
> >> +
> >> +    if ( max > UINT_MAX )
> >> +        max = UINT_MAX;
> > 
> > Can't you use a min helper here? It would be clear IMHO.
> 
> I'm afraid there's no min() available that would work (build) equally
> in both libxc and hypervisor context.

In that case:

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel