bfd: Always check compressed sections with the corrupt size

Message ID 20211104133826.59372-1-hjl.tools@gmail.com
State New
Headers show
Series
  • bfd: Always check compressed sections with the corrupt size
Related show

Commit Message

H.J. Lu via Binutils Nov. 4, 2021, 1:38 p.m.
Always check compressed sections with the corrupt size for non-MMO
files.  Skip MMO files for compress_status == COMPRESS_SECTION_NONE
since MMO has special handling for COMPRESS_SECTION_NONE.

I am checking in this.

H.J.
---
	PR binutils/28530
	* compress.c (bfd_get_full_section_contents): Always check
	compressed sections with the corrupt size.
---
 bfd/compress.c | 50 +++++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 23 deletions(-)

-- 
2.33.1

Comments

H.J. Lu via Binutils Nov. 4, 2021, 10:16 p.m. | #1
On Thu, Nov 04, 2021 at 06:38:26AM -0700, H.J. Lu wrote:
> Always check compressed sections with the corrupt size for non-MMO

> files.  Skip MMO files for compress_status == COMPRESS_SECTION_NONE

> since MMO has special handling for COMPRESS_SECTION_NONE.

> 

> I am checking in this.


This doesn't look correct to me.  When decompressing you can read a
section that is larger than the file size.

-- 
Alan Modra
Australia Development Lab, IBM
Hans-Peter Nilsson Nov. 5, 2021, 2:40 a.m. | #2
On Thu, 4 Nov 2021, H.J. Lu via Binutils wrote:

> Always check compressed sections with the corrupt size for non-MMO

> files.  Skip MMO files for compress_status == COMPRESS_SECTION_NONE

> since MMO has special handling for COMPRESS_SECTION_NONE.

>

> I am checking in this.


Did I miss the approval?

> H.J.

> ---

> 	PR binutils/28530

> 	* compress.c (bfd_get_full_section_contents): Always check

> 	compressed sections with the corrupt size.

> ---

>  bfd/compress.c | 50 +++++++++++++++++++++++++++-----------------------

>  1 file changed, 27 insertions(+), 23 deletions(-)

>

> diff --git a/bfd/compress.c b/bfd/compress.c

> index 4a2ada3e3eb..a3adb8d8250 100644

> --- a/bfd/compress.c

> +++ b/bfd/compress.c

> @@ -232,6 +232,7 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)

>    bfd_size_type save_rawsize;

>    bfd_byte *compressed_buffer;

>    unsigned int compression_header_size;

> +  ufile_ptr filesize;

>

>    if (abfd->direction != write_direction && sec->rawsize != 0)

>      sz = sec->rawsize;

> @@ -243,34 +244,37 @@ bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)

>        return true;

>      }

>

> +  filesize = bfd_get_file_size (abfd);

> +  if (filesize > 0

> +      && filesize < sz

> +      /* PR 24753: Linker created sections can be larger than

> +	 the file size, eg if they are being used to hold stubs.  */

> +      && (bfd_section_flags (sec) & SEC_LINKER_CREATED) == 0

> +      /* PR 24753: Sections which have no content should also be

> +	 excluded as they contain no size on disk.  */

> +      && (bfd_section_flags (sec) & SEC_HAS_CONTENTS) != 0

> +      /* PR 28530: Check compressed sections with the corrupt size.  */

> +      && (sec->compress_status != COMPRESS_SECTION_NONE

> +      /* The MMO file format supports its own special compression

> +	 technique, but it uses COMPRESS_SECTION_NONE when loading

> +	 a section's contents.  */

> +	  || bfd_get_flavour (abfd) != bfd_target_mmo_flavour))


Looking at the BFD flavor seems like the wrong thing to me.
Perhaps add a (defaulted) BFD field or hook if needed?

Also, ELF section contents in the mmo format is embedded, not
compressed.  "Coded" if you will, compared to the file contents;
not a straight blob of bytes, rather escaped sequences.  Also,
for future reference, it's "mmo", not "MMO".

brgds, H-P

Patch

diff --git a/bfd/compress.c b/bfd/compress.c
index 4a2ada3e3eb..a3adb8d8250 100644
--- a/bfd/compress.c
+++ b/bfd/compress.c
@@ -232,6 +232,7 @@  bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
   bfd_size_type save_rawsize;
   bfd_byte *compressed_buffer;
   unsigned int compression_header_size;
+  ufile_ptr filesize;
 
   if (abfd->direction != write_direction && sec->rawsize != 0)
     sz = sec->rawsize;
@@ -243,34 +244,37 @@  bfd_get_full_section_contents (bfd *abfd, sec_ptr sec, bfd_byte **ptr)
       return true;
     }
 
+  filesize = bfd_get_file_size (abfd);
+  if (filesize > 0
+      && filesize < sz
+      /* PR 24753: Linker created sections can be larger than
+	 the file size, eg if they are being used to hold stubs.  */
+      && (bfd_section_flags (sec) & SEC_LINKER_CREATED) == 0
+      /* PR 24753: Sections which have no content should also be
+	 excluded as they contain no size on disk.  */
+      && (bfd_section_flags (sec) & SEC_HAS_CONTENTS) != 0
+      /* PR 28530: Check compressed sections with the corrupt size.  */
+      && (sec->compress_status != COMPRESS_SECTION_NONE
+      /* The MMO file format supports its own special compression
+	 technique, but it uses COMPRESS_SECTION_NONE when loading
+	 a section's contents.  */
+	  || bfd_get_flavour (abfd) != bfd_target_mmo_flavour))
+    {
+      /* PR 24708: Avoid attempts to allocate a ridiculous amount
+	 of memory.  */
+      bfd_set_error (bfd_error_file_truncated);
+      _bfd_error_handler
+	/* xgettext:c-format */
+	(_("error: %pB(%pA) section size (%#" PRIx64 " bytes) is larger than file size (%#" PRIx64 " bytes)"),
+	 abfd, sec, (uint64_t) sz, (uint64_t) filesize);
+      return false;
+    }
+
   switch (sec->compress_status)
     {
     case COMPRESS_SECTION_NONE:
       if (p == NULL)
 	{
-	  ufile_ptr filesize = bfd_get_file_size (abfd);
-	  if (filesize > 0
-	      && filesize < sz
-	      /* PR 24753: Linker created sections can be larger than
-		 the file size, eg if they are being used to hold stubs.  */
-	      && (bfd_section_flags (sec) & SEC_LINKER_CREATED) == 0
-	      /* PR 24753: Sections which have no content should also be
-		 excluded as they contain no size on disk.  */
-	      && (bfd_section_flags (sec) & SEC_HAS_CONTENTS) != 0
-	      /* The MMO file format supports its own special compression
-		 technique, but it uses COMPRESS_SECTION_NONE when loading
-		 a section's contents.  */
-	      && bfd_get_flavour (abfd) != bfd_target_mmo_flavour)
-	    {
-	      /* PR 24708: Avoid attempts to allocate a ridiculous amount
-		 of memory.  */
-	      bfd_set_error (bfd_error_file_truncated);
-	      _bfd_error_handler
-		/* xgettext:c-format */
-		(_("error: %pB(%pA) section size (%#" PRIx64 " bytes) is larger than file size (%#" PRIx64 " bytes)"),
-		 abfd, sec, (uint64_t) sz, (uint64_t) filesize);
-	      return false;
-	    }
 	  p = (bfd_byte *) bfd_malloc (sz);
 	  if (p == NULL)
 	    {