Re: [PATCH] libdw: check memory access in get_(u|s)leb128

[Mark Wielaard]

Hi Aleksei,

On Wed, Jan 25, 2023 at 04:05:30PM +0000, Aleksei Vetrov via Elfutils-devel 
wrote:
> From: Aleksei Vetrov <vvv...@google.com>
> 
> __libdw_get_uleb128 and __libdw_get_sleb128 should check if addrp has
> already reached the end before unrolling the first step. It is done by
> moving __libdw_max_len to the beginning of the function, which already
> has all the checks.

I did some performance tests and couldn't find any significant change.
Even with my other extra checks added in libdw and readelf.

One question about the sleb128 case though:

>  static inline int64_t
>  __libdw_get_sleb128 (const unsigned char **addrp, const unsigned char *end)
>  {
> +  const size_t max = __libdw_max_len_sleb128 (*addrp, end);
>    /* Do the work in an unsigned type, but use implementation-defined
>       behavior to cast to signed on return.  This avoids some undefined
>       behavior when shifting.  */
> @@ -131,9 +133,9 @@ __libdw_get_sleb128 (const unsigned char **addrp, const 
> unsigned char *end)
>  
>    /* Unroll the first step to help the compiler optimize
>       for the common single-byte case.  */
> -  get_sleb128_step (acc, *addrp, 0);
> +  if (likely (max > 0))
> +    get_sleb128_step (acc, *addrp, 0);
>  
> -  const size_t max = __libdw_max_len_sleb128 (*addrp - 1, end);
>    for (size_t i = 1; i < max; ++i)
>      get_sleb128_step (acc, *addrp, i);
>    if (*addrp == end)

But what about the case where *addrp > end?
After this code we will do:

  /* There might be one extra byte.  */
  unsigned char b = **addrp;
  ++*addrp;

So I think we want to catch that too.  Easiest imho seems to move (and
invert) the max check immediately after calculating max:

diff --git a/libdw/memory-access.h b/libdw/memory-access.h
index 1cac6af3..72348623 100644
--- a/libdw/memory-access.h
+++ b/libdw/memory-access.h
@@ -126,6 +126,9 @@ static inline int64_t
 __libdw_get_sleb128 (const unsigned char **addrp, const unsigned char *end)
 {
   const size_t max = __libdw_max_len_sleb128 (*addrp, end);
+  if (unlikely (max == 0))
+    return INT64_MAX;
+
   /* Do the work in an unsigned type, but use implementation-defined
      behavior to cast to signed on return.  This avoids some undefined
      behavior when shifting.  */
@@ -133,8 +136,7 @@ __libdw_get_sleb128 (const unsigned char **addrp, const 
unsigned char *end)
 
   /* Unroll the first step to help the compiler optimize
      for the common single-byte case.  */
-  if (likely (max > 0))
-    get_sleb128_step (acc, *addrp, 0);
+  get_sleb128_step (acc, *addrp, 0);
 
   for (size_t i = 1; i < max; ++i)
     get_sleb128_step (acc, *addrp, i);

Cheers,

Mark