[PATCH] libsframe: adjust an incorrect check in flip_sframe

[PATCH] libsframe: adjust an incorrect check in flip_sframe

[Indu Bhagat]

Hello,

While testing the endian flipping code in libsframe, I tried a cross build with
--target=x86_64-linux on a big-endian system (ppc64), and I ran into some
SFrame testsuite failures:

    FAIL: SFrame Simple link
    FAIL: SFrame for plt0 and pltN

The reason for failure is explained below in the commit log.  This patch fixes
the issue.

Testing notes:
- Reg tested native and cross builds on x86_64 and aarch64 (checked binutils,
  ld, gas, libctf, libsframe).
- try bot shows no new regressions.
- Tested ld, gas with a cross build: --target=x86_64-linux on ppc64 host.  The
  two testcase failures are resolved with this patch.

  Before the patch:
      FAIL: SFrame Simple link
      FAIL: SFrame for plt0 and pltN

  === ld Summary ===

  # of expected passes            1651
  # of unexpected failures        2
  # of expected failures          3
  # of untested testcases         26
  # of unsupported tests          134

  After the patch:
			      
  === ld Summary ===

  # of expected passes            1653
  # of expected failures          3
  # of untested testcases         26
  # of unsupported tests          134

OK for master and binutils-2_40-branch ?

Thanks

--------------------------------------

When sframe_encoder_write needs to flip the buffer containing the SFrame
section before writing, it is not necessary that the SFrame FDES are in
the order of their sfde_func_start_fre_off.  On the contrary, SFrame
FDEs will be sorted in the order of their start address.  So, remove
this incorrect assumption which is basically assuming that the last
sfde_func_start_fre_off seen will help determine the end of the flipped
buffer.

The function now keeps track of the bytes_flipped and then compares it with
the expected value.  Also, added two more checks at appropriate places:
 - check that the SFrame FDE read is within bounds
 - check that the SFrame FRE read is within bounds

ChangeLog:

	* libsframe/sframe.c (flip_sframe): Adjust an incorrect check.
	Add other checks to ensure reads are within the buffer size.
---
 libsframe/sframe.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index 4aada1a25e0..d206780289a 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -401,7 +401,10 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
   unsigned int fre_type = 0;
   uint32_t fre_offset = 0;
   size_t esz = 0;
+  size_t hdrsz = 0;
   int err = 0;
+  /* For error checking.  */
+  size_t bytes_flipped = 0;
 
   /* Header must be in host endianness at this time.  */
   ihp = (sframe_header *)frame_buf;
@@ -411,14 +414,18 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
 
   /* The contents of the SFrame header are safe to read.  Get the number of
      FDEs and the first FDE in the buffer.  */
+  hdrsz = sframe_get_hdr_size (ihp);
   num_fdes = ihp->sfh_num_fdes;
-  fdes = frame_buf + sframe_get_hdr_size (ihp) + ihp->sfh_fdeoff;
+  fdes = frame_buf + hdrsz + ihp->sfh_fdeoff;
   fdep = (sframe_func_desc_entry *)fdes;
 
   j = 0;
   prev_frep_index = 0;
   for (i = 0; i < num_fdes; fdep++, i++)
     {
+      if ((char*)fdep >= (frame_buf + buf_size))
+	goto bad;
+
       if (to_foreign)
 	{
 	  num_fres = fdep->sfde_func_num_fres;
@@ -427,6 +434,7 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
 	}
 
       flip_fde (fdep);
+      bytes_flipped += sizeof (sframe_func_desc_entry);
 
       if (!to_foreign)
 	{
@@ -441,20 +449,16 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
 	{
 	  if (flip_fre (fp, fre_type, &esz))
 	    goto bad;
+	  bytes_flipped += esz;
 
-	  if (esz == 0)
+	  if (esz == 0 || esz > buf_size)
 	    goto bad;
 	  fp += esz;
 	}
       prev_frep_index = j;
     }
-  /* All FREs must have been endian flipped by now.  */
-  if (j != ihp->sfh_num_fres)
-    goto bad;
-  /* Contents, if any, must have been processed by now.
-     Recall that .sframe section with just a SFrame header may be generated by
-     GAS if no SFrame FDEs were found for the input file.  */
-  if (ihp->sfh_num_fres && ((frame_buf + buf_size) != (void*)fp))
+  /* All FDEs and FREs must have been endian flipped by now.  */
+  if ((j != ihp->sfh_num_fres) || (bytes_flipped != (buf_size - hdrsz)))
     goto bad;
 
   /* Success.  */
-- 
2.37.2

Re: [PATCH] libsframe: adjust an incorrect check in flip_sframe

[Indu Bhagat]

On 1/3/23 10:56 PM, Indu Bhagat via Binutils wrote:
> Hello,
> 
> While testing the endian flipping code in libsframe, I tried a cross build with
> --target=x86_64-linux on a big-endian system (ppc64), and I ran into some
> SFrame testsuite failures:
> 
>      FAIL: SFrame Simple link
>      FAIL: SFrame for plt0 and pltN
> 
> The reason for failure is explained below in the commit log.  This patch fixes
> the issue.
> 
> Testing notes:
> - Reg tested native and cross builds on x86_64 and aarch64 (checked binutils,
>    ld, gas, libctf, libsframe).
> - try bot shows no new regressions.
> - Tested ld, gas with a cross build: --target=x86_64-linux on ppc64 host.  The
>    two testcase failures are resolved with this patch.
> 
>    Before the patch:
>        FAIL: SFrame Simple link
>        FAIL: SFrame for plt0 and pltN
> 
>    === ld Summary ===
> 
>    # of expected passes            1651
>    # of unexpected failures        2
>    # of expected failures          3
>    # of untested testcases         26
>    # of unsupported tests          134
> 
>    After the patch:
> 			
>    === ld Summary ===
> 
>    # of expected passes            1653
>    # of expected failures          3
>    # of untested testcases         26
>    # of unsupported tests          134
> 
> OK for master and binutils-2_40-branch ?
> 

I have pushed this to master.

Is this OK for binutils-2_40-branch ? Without this patch, there will be 
undesirable effects (failing ld SFrame tests, also meaning limited 
SFrame functionality) in some cross-builds.

Thanks
Indu

> Thanks
> 
> --------------------------------------
> 
> When sframe_encoder_write needs to flip the buffer containing the SFrame
> section before writing, it is not necessary that the SFrame FDES are in
> the order of their sfde_func_start_fre_off.  On the contrary, SFrame
> FDEs will be sorted in the order of their start address.  So, remove
> this incorrect assumption which is basically assuming that the last
> sfde_func_start_fre_off seen will help determine the end of the flipped
> buffer.
> 
> The function now keeps track of the bytes_flipped and then compares it with
> the expected value.  Also, added two more checks at appropriate places:
>   - check that the SFrame FDE read is within bounds
>   - check that the SFrame FRE read is within bounds
> 
> ChangeLog:
> 
> 	* libsframe/sframe.c (flip_sframe): Adjust an incorrect check.
> 	Add other checks to ensure reads are within the buffer size.
> ---
>   libsframe/sframe.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
> index 4aada1a25e0..d206780289a 100644
> --- a/libsframe/sframe.c
> +++ b/libsframe/sframe.c
> @@ -401,7 +401,10 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
>     unsigned int fre_type = 0;
>     uint32_t fre_offset = 0;
>     size_t esz = 0;
> +  size_t hdrsz = 0;
>     int err = 0;
> +  /* For error checking.  */
> +  size_t bytes_flipped = 0;
>   
>     /* Header must be in host endianness at this time.  */
>     ihp = (sframe_header *)frame_buf;
> @@ -411,14 +414,18 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
>   
>     /* The contents of the SFrame header are safe to read.  Get the number of
>        FDEs and the first FDE in the buffer.  */
> +  hdrsz = sframe_get_hdr_size (ihp);
>     num_fdes = ihp->sfh_num_fdes;
> -  fdes = frame_buf + sframe_get_hdr_size (ihp) + ihp->sfh_fdeoff;
> +  fdes = frame_buf + hdrsz + ihp->sfh_fdeoff;
>     fdep = (sframe_func_desc_entry *)fdes;
>   
>     j = 0;
>     prev_frep_index = 0;
>     for (i = 0; i < num_fdes; fdep++, i++)
>       {
> +      if ((char*)fdep >= (frame_buf + buf_size))
> +	goto bad;
> +
>         if (to_foreign)
>   	{
>   	  num_fres = fdep->sfde_func_num_fres;
> @@ -427,6 +434,7 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
>   	}
>   
>         flip_fde (fdep);
> +      bytes_flipped += sizeof (sframe_func_desc_entry);
>   
>         if (!to_foreign)
>   	{
> @@ -441,20 +449,16 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
>   	{
>   	  if (flip_fre (fp, fre_type, &esz))
>   	    goto bad;
> +	  bytes_flipped += esz;
>   
> -	  if (esz == 0)
> +	  if (esz == 0 || esz > buf_size)
>   	    goto bad;
>   	  fp += esz;
>   	}
>         prev_frep_index = j;
>       }
> -  /* All FREs must have been endian flipped by now.  */
> -  if (j != ihp->sfh_num_fres)
> -    goto bad;
> -  /* Contents, if any, must have been processed by now.
> -     Recall that .sframe section with just a SFrame header may be generated by
> -     GAS if no SFrame FDEs were found for the input file.  */
> -  if (ihp->sfh_num_fres && ((frame_buf + buf_size) != (void*)fp))
> +  /* All FDEs and FREs must have been endian flipped by now.  */
> +  if ((j != ihp->sfh_num_fres) || (bytes_flipped != (buf_size - hdrsz)))
>       goto bad;
>   
>     /* Success.  */
> 

Re: [PATCH] libsframe: adjust an incorrect check in flip_sframe

[Nick Clifton]

Hi Indu,

> Is this OK for binutils-2_40-branch ? Without this patch, there will be undesirable effects (failing ld SFrame tests, also meaning limited SFrame functionality) in some 
> cross-builds.

Yes - please apply to the branch.

Cheers
   Nick