diff mbox series

[04/10] libsframe: validate FDE/FRE offsets in SFrame header

Message ID 20260827224146.3391610-5-ibhagatgnu@gmail.com
State New
Headers
Series fix two PRs and guard against bad data |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Indu Bhagat Aug. 27, 2026, 10:41 p.m. UTC 
  Currently, the check for preamble magic, version, flags was done in
sframe_decode (), while sframe_header_sanity_check_p () performed
minimal sanity check of sfh_fdeoff <= sfh_freoff.

There is need to validate header section offsets and length fields
(sfh_fdeoff, sfh_freoff, sfh_fre_len, sfh_num_fdes) against the
caller-supplied buffer size buf_size. If unchecked, a crafted .sframe
section with invalid header offsets may cause out-of-bounds reads.

Update sframe_header_sanity_check_p() to accept buf_size and validate:
  - Header size hdrsz <= buf_size
  - hdrsz + sfh_fdeoff + num_fdes * fde_entry_size <= sfh_freoff
  - hdrsz + sfh_freoff + sfh_fre_len <= buf_size

Update all call sites of sframe_header_sanity_check_p() to pass the
buffer size.

This fixes a subset of issues raised in PR libsframe/34273

An alternate fix was suggested by Naveed Khan earlier on the mailing
list.  This commit is an adaptation of the same.

Suggested-by: Naveed Khan <naveed@digiscrypt.com>
---
 libsframe/sframe.c | 43 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 6 deletions(-)

Comments

Jens Remus Sept. 7, 2026, 12:37 p.m. UTC | #1 
On 8/28/2026 12:41 AM, Indu Bhagat wrote:
> Currently, the check for preamble magic, version, flags was done in
> sframe_decode (), while sframe_header_sanity_check_p () performed
> minimal sanity check of sfh_fdeoff <= sfh_freoff.
> 
> There is need to validate header section offsets and length fields
> (sfh_fdeoff, sfh_freoff, sfh_fre_len, sfh_num_fdes) against the
> caller-supplied buffer size buf_size. If unchecked, a crafted .sframe
> section with invalid header offsets may cause out-of-bounds reads.
> 
> Update sframe_header_sanity_check_p() to accept buf_size and validate:
>   - Header size hdrsz <= buf_size
>   - hdrsz + sfh_fdeoff + num_fdes * fde_entry_size <= sfh_freoff
>   - hdrsz + sfh_freoff + sfh_fre_len <= buf_size
> 
> Update all call sites of sframe_header_sanity_check_p() to pass the
> buffer size.
> 
> This fixes a subset of issues raised in PR libsframe/34273
> 
> An alternate fix was suggested by Naveed Khan earlier on the mailing
> list.  This commit is an adaptation of the same.
> 
> Suggested-by: Naveed Khan <naveed@digiscrypt.com>
> ---
>  libsframe/sframe.c | 43 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)

Reviewed-by: Jens Remus <jremus@linux.ibm.com>

> diff --git a/libsframe/sframe.c b/libsframe/sframe.c

> @@ -358,8 +358,11 @@ flip_fde_attr_v3 (char *buf, size_t buf_size)
>  /* Check if SFrame header has valid data.  */
>  
>  static bool
> -sframe_header_sanity_check_p (const sframe_header *hp)
> +sframe_header_sanity_check_p (const sframe_header *hp, size_t buf_size)
>  {
> +  if (buf_size < sizeof (sframe_header))
> +    return false;
> +
>    /* Check preamble is valid.  */
>    if (hp->sfh_preamble.sfp_magic != SFRAME_MAGIC
>        || (hp->sfh_preamble.sfp_version != SFRAME_VERSION_1
> @@ -377,10 +380,38 @@ sframe_header_sanity_check_p (const sframe_header *hp)
>    if (hp->sfh_preamble.sfp_flags & ~valid_flags)
>      return false;
>  
> -  /* Check offsets are valid.  */
> +  size_t hdrsz = sframe_get_hdr_size (hp);
> +  if (buf_size < hdrsz)
> +    return false;
> +
> +  /* Check offsets are within buf_size and sensible.  */
> +  size_t fde_entry_size = (hp->sfh_preamble.sfp_version == SFRAME_VERSION_3)
> +			   ? sizeof (sframe_func_desc_idx_v3)
> +			   : sizeof (sframe_func_desc_entry_v2);
> +
> +  if (hp->sfh_num_fdes > (SIZE_MAX / fde_entry_size))
> +    return false;
> +
> +  size_t fidx_size = hp->sfh_num_fdes * fde_entry_size;
> +
>    if (hp->sfh_fdeoff > hp->sfh_freoff)
>      return false;
>  
> +  if (hp->sfh_fdeoff > buf_size - hdrsz)
> +    return false;
> +
> +  if (fidx_size > buf_size - hdrsz - hp->sfh_fdeoff)
> +    return false;
> +
> +  if (hp->sfh_freoff > buf_size - hdrsz)
> +    return false;
> +
> +  if (hp->sfh_fre_len > buf_size - hdrsz - hp->sfh_freoff)
> +    return false;
> +
> +  if (hp->sfh_fdeoff + fidx_size > hp->sfh_freoff)
> +    return false;
> +

Would it make sense to reorder/regroup as follows, to keep related
overflow checks together?

  if (hp->sfh_num_fdes > (SIZE_MAX / fde_entry_size))
    return false;
  size_t fidx_size = hp->sfh_num_fdes * fde_entry_size;

  if (hp->sfh_fdeoff > buf_size - hdrsz)
    return false;
  if (fidx_size > buf_size - hdrsz - hp->sfh_fdeoff)
    return false;

  if (hp->sfh_freoff > buf_size - hdrsz)
    return false;
  if (hp->sfh_fre_len > buf_size - hdrsz - hp->sfh_freoff)
    return false;

  if (hp->sfh_fdeoff > hp->sfh_freoff)
    return false;
  if (hp->sfh_fdeoff + fidx_size > hp->sfh_freoff)
    return false;

Or even shorter:

  if (hp->sfh_num_fdes > (SIZE_MAX / fde_entry_size))
    return false;
  size_t fidx_size = hp->sfh_num_fdes * fde_entry_size;

  if ((hp->sfh_fdeoff > buf_size - hdrsz)
      || (fidx_size > buf_size - hdrsz - hp->sfh_fdeoff))
    return false;

  if ((hp->sfh_freoff > buf_size - hdrsz)
      || (hp->sfh_fre_len > buf_size - hdrsz - hp->sfh_freoff))
    return false;

  if ((hp->sfh_fdeoff > hp->sfh_freoff)
      || (hp->sfh_fdeoff + fidx_size > hp->sfh_freoff))
    return false;

>    return true;
>  }
>  
> @@ -733,7 +764,7 @@ flip_sframe_fdes_with_fres_v2 (char *frame_buf, size_t buf_size,
>    /* Header must be in host endianness at this time.  */
>    const sframe_header *ihp = (sframe_header *)frame_buf;
>  
> -  if (!sframe_header_sanity_check_p (ihp))
> +  if (!sframe_header_sanity_check_p (ihp, buf_size))
>      return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
>  
>    /* The contents of the SFrame header are safe to read.  Get the number of
> @@ -829,7 +860,7 @@ flip_sframe_fdes_with_fres_v3 (char *frame_buf, size_t buf_size,
>    /* Header must be in host endianness at this time.  */
>    const sframe_header *ihp = (sframe_header *)frame_buf;
>  
> -  if (!sframe_header_sanity_check_p (ihp))
> +  if (!sframe_header_sanity_check_p (ihp, buf_size))
>      return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
>  
>    /* The contents of the SFrame header are safe to read.  Get the number of
> @@ -917,7 +948,7 @@ flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
>  
>    /* Header must be in host endianness at this time.  */
>    const sframe_header *ihp = (sframe_header *)frame_buf;
> -  if (!sframe_header_sanity_check_p (ihp))
> +  if (!sframe_header_sanity_check_p (ihp, buf_size))
>      return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
>    uint8_t ver = ihp->sfh_preamble.sfp_version;
>  
> @@ -1482,7 +1513,7 @@ sframe_decode (const char *sf_buf, size_t sf_size, int *errp)
>    dctx->sfd_header = *(sframe_header *) frame_buf;
>    /* Validate the contents of SFrame header.  */
>    dhp = &dctx->sfd_header;
> -  if (!sframe_header_sanity_check_p (dhp))
> +  if (!sframe_header_sanity_check_p (dhp, sf_size))
>      {
>        sframe_ret_set_errno (errp, SFRAME_ERR_BUF_INVAL);
>        goto decode_fail_free;

Regards,
Jens
diff mbox series

Patch

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index d50a9e5689b..4742e4550c2 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -358,8 +358,11 @@  flip_fde_attr_v3 (char *buf, size_t buf_size)
 /* Check if SFrame header has valid data.  */
 
 static bool
-sframe_header_sanity_check_p (const sframe_header *hp)
+sframe_header_sanity_check_p (const sframe_header *hp, size_t buf_size)
 {
+  if (buf_size < sizeof (sframe_header))
+    return false;
+
   /* Check preamble is valid.  */
   if (hp->sfh_preamble.sfp_magic != SFRAME_MAGIC
       || (hp->sfh_preamble.sfp_version != SFRAME_VERSION_1
@@ -377,10 +380,38 @@  sframe_header_sanity_check_p (const sframe_header *hp)
   if (hp->sfh_preamble.sfp_flags & ~valid_flags)
     return false;
 
-  /* Check offsets are valid.  */
+  size_t hdrsz = sframe_get_hdr_size (hp);
+  if (buf_size < hdrsz)
+    return false;
+
+  /* Check offsets are within buf_size and sensible.  */
+  size_t fde_entry_size = (hp->sfh_preamble.sfp_version == SFRAME_VERSION_3)
+			   ? sizeof (sframe_func_desc_idx_v3)
+			   : sizeof (sframe_func_desc_entry_v2);
+
+  if (hp->sfh_num_fdes > (SIZE_MAX / fde_entry_size))
+    return false;
+
+  size_t fidx_size = hp->sfh_num_fdes * fde_entry_size;
+
   if (hp->sfh_fdeoff > hp->sfh_freoff)
     return false;
 
+  if (hp->sfh_fdeoff > buf_size - hdrsz)
+    return false;
+
+  if (fidx_size > buf_size - hdrsz - hp->sfh_fdeoff)
+    return false;
+
+  if (hp->sfh_freoff > buf_size - hdrsz)
+    return false;
+
+  if (hp->sfh_fre_len > buf_size - hdrsz - hp->sfh_freoff)
+    return false;
+
+  if (hp->sfh_fdeoff + fidx_size > hp->sfh_freoff)
+    return false;
+
   return true;
 }
 
@@ -733,7 +764,7 @@  flip_sframe_fdes_with_fres_v2 (char *frame_buf, size_t buf_size,
   /* Header must be in host endianness at this time.  */
   const sframe_header *ihp = (sframe_header *)frame_buf;
 
-  if (!sframe_header_sanity_check_p (ihp))
+  if (!sframe_header_sanity_check_p (ihp, buf_size))
     return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
 
   /* The contents of the SFrame header are safe to read.  Get the number of
@@ -829,7 +860,7 @@  flip_sframe_fdes_with_fres_v3 (char *frame_buf, size_t buf_size,
   /* Header must be in host endianness at this time.  */
   const sframe_header *ihp = (sframe_header *)frame_buf;
 
-  if (!sframe_header_sanity_check_p (ihp))
+  if (!sframe_header_sanity_check_p (ihp, buf_size))
     return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
 
   /* The contents of the SFrame header are safe to read.  Get the number of
@@ -917,7 +948,7 @@  flip_sframe (char *frame_buf, size_t buf_size, uint32_t to_foreign)
 
   /* Header must be in host endianness at this time.  */
   const sframe_header *ihp = (sframe_header *)frame_buf;
-  if (!sframe_header_sanity_check_p (ihp))
+  if (!sframe_header_sanity_check_p (ihp, buf_size))
     return sframe_set_errno (&err, SFRAME_ERR_BUF_INVAL);
   uint8_t ver = ihp->sfh_preamble.sfp_version;
 
@@ -1482,7 +1513,7 @@  sframe_decode (const char *sf_buf, size_t sf_size, int *errp)
   dctx->sfd_header = *(sframe_header *) frame_buf;
   /* Validate the contents of SFrame header.  */
   dhp = &dctx->sfd_header;
-  if (!sframe_header_sanity_check_p (dhp))
+  if (!sframe_header_sanity_check_p (dhp, sf_size))
     {
       sframe_ret_set_errno (errp, SFRAME_ERR_BUF_INVAL);
       goto decode_fail_free;