[PATCH v3] ifaddrs: Get rid of alloca

[PATCH v3] ifaddrs: Get rid of alloca

[Joe Simmons-Talbott]

Use scratch_buffer and malloc rather than alloca to avoid potential stack
overflows.
---
Changes to v2:
  * Initialize the scratch_buffer earlier to avoid memory access errors
    when calling scratch_buffer_free for failure cases.

Changes to v1:
  * in __netlink_request use an 8kb buffer size and malloc rather than a
    scratch_buffer.

    Suggested-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>

 sysdeps/unix/sysv/linux/ifaddrs.c | 51 +++++++++++++++----------------
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
index 184ee224cb..f62510811a 100644
--- a/sysdeps/unix/sysv/linux/ifaddrs.c
+++ b/sysdeps/unix/sysv/linux/ifaddrs.c
@@ -16,13 +16,13 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <alloca.h>
 #include <assert.h>
 #include <errno.h>
 #include <ifaddrs.h>
 #include <net/if.h>
 #include <netinet/in.h>
 #include <netpacket/packet.h>
+#include <scratch_buffer.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -122,6 +122,13 @@ __netlink_sendreq (struct netlink_handle *h, int type)
 }
 
 
+static void
+ifree (char **ptr)
+{
+  free (*ptr);
+}
+
+
 int
 __netlink_request (struct netlink_handle *h, int type)
 {
@@ -131,26 +138,14 @@ __netlink_request (struct netlink_handle *h, int type)
   ssize_t read_len;
   bool done = false;
 
-#ifdef PAGE_SIZE
-  /* Help the compiler optimize out the malloc call if PAGE_SIZE
-     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
-  const size_t buf_size = PAGE_SIZE;
-#else
-  const size_t buf_size = __getpagesize ();
-#endif
-  bool use_malloc = false;
-  char *buf;
-
-  if (__libc_use_alloca (buf_size))
-    buf = alloca (buf_size);
-  else
-    {
-      buf = malloc (buf_size);
-      if (buf != NULL)
-	use_malloc = true;
-      else
-	goto out_fail;
-    }
+  /* Netlink requires that user buffer needs to be either 8kb or page size
+     (whichever is bigger), however this has been changed over time and now
+     8Kb is sufficient (check NLMSG_DEFAULT_SIZE on Linux
+     linux/include/linux/netlink.h).  */
+  const size_t buf_size = 8192;
+  char *buf __attribute__ ((__cleanup__ (ifree))) = malloc (buf_size);
+  if (buf == NULL)
+    return -1;
 
   struct iovec iov = { buf, buf_size };
 
@@ -229,13 +224,9 @@ __netlink_request (struct netlink_handle *h, int type)
       h->end_ptr = nlm_next;
     }
 
-  if (use_malloc)
-    free (buf);
   return 0;
 
 out_fail:
-  if (use_malloc)
-    free (buf);
   return -1;
 }
 
@@ -324,6 +315,8 @@ getifaddrs_internal (struct ifaddrs **ifap)
   char *ifa_data_ptr;	/* Pointer to the unused part of memory for
 				ifa_data.  */
   int result = 0;
+  struct scratch_buffer buf;
+  scratch_buffer_init (&buf);
 
   *ifap = NULL;
 
@@ -425,7 +418,12 @@ getifaddrs_internal (struct ifaddrs **ifap)
     }
 
   /* Table for mapping kernel index to entry in our list.  */
-  map_newlink_data = alloca (newlink * sizeof (int));
+  if (!scratch_buffer_set_array_size (&buf, 1, newlink * sizeof (int)))
+    {
+      result = -1;
+      goto exit_free;
+    }
+  map_newlink_data = buf.data;
   memset (map_newlink_data, '\xff', newlink * sizeof (int));
 
   ifa_data_ptr = (char *) &ifas[newlink + newaddr];
@@ -820,6 +818,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
  exit_free:
   __netlink_free_handle (&nh);
   __netlink_close (&nh);
+  scratch_buffer_free (&buf);
 
   return result;
 }
-- 
2.39.2

Re: [PATCH v3] ifaddrs: Get rid of alloca

[Adhemerval Zanella Netto]

On 30/05/23 12:25, Joe Simmons-Talbott via Libc-alpha wrote:
> Use scratch_buffer and malloc rather than alloca to avoid potential stack
> overflows.
> ---
> Changes to v2:
>   * Initialize the scratch_buffer earlier to avoid memory access errors
>     when calling scratch_buffer_free for failure cases.
> 
> Changes to v1:
>   * in __netlink_request use an 8kb buffer size and malloc rather than a
>     scratch_buffer.
> 
>     Suggested-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
> 
>  sysdeps/unix/sysv/linux/ifaddrs.c | 51 +++++++++++++++----------------
>  1 file changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
> index 184ee224cb..f62510811a 100644
> --- a/sysdeps/unix/sysv/linux/ifaddrs.c
> +++ b/sysdeps/unix/sysv/linux/ifaddrs.c
> @@ -16,13 +16,13 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> -#include <alloca.h>
>  #include <assert.h>
>  #include <errno.h>
>  #include <ifaddrs.h>
>  #include <net/if.h>
>  #include <netinet/in.h>
>  #include <netpacket/packet.h>
> +#include <scratch_buffer.h>
>  #include <stdbool.h>
>  #include <stdint.h>
>  #include <stdlib.h>
> @@ -122,6 +122,13 @@ __netlink_sendreq (struct netlink_handle *h, int type)
>  }
>  
>  
> +static void
> +ifree (char **ptr)
> +{
> +  free (*ptr);
> +}
> +
> +
>  int
>  __netlink_request (struct netlink_handle *h, int type)
>  {
> @@ -131,26 +138,14 @@ __netlink_request (struct netlink_handle *h, int type)
>    ssize_t read_len;
>    bool done = false;
>  
> -#ifdef PAGE_SIZE
> -  /* Help the compiler optimize out the malloc call if PAGE_SIZE
> -     is constant and smaller or equal to PTHREAD_STACK_MIN/4.  */
> -  const size_t buf_size = PAGE_SIZE;
> -#else
> -  const size_t buf_size = __getpagesize ();
> -#endif
> -  bool use_malloc = false;
> -  char *buf;
> -
> -  if (__libc_use_alloca (buf_size))
> -    buf = alloca (buf_size);
> -  else
> -    {
> -      buf = malloc (buf_size);
> -      if (buf != NULL)
> -	use_malloc = true;
> -      else
> -	goto out_fail;
> -    }
> +  /* Netlink requires that user buffer needs to be either 8kb or page size
> +     (whichever is bigger), however this has been changed over time and now
> +     8Kb is sufficient (check NLMSG_DEFAULT_SIZE on Linux
> +     linux/include/linux/netlink.h).  */
> +  const size_t buf_size = 8192;
> +  char *buf __attribute__ ((__cleanup__ (ifree))) = malloc (buf_size);
> +  if (buf == NULL)
> +    return -1;

With the cleanup handler you can remove all the 'goto out_fail' and just
return -1.

diff --git a/sysdeps/unix/sysv/linux/ifaddrs.c b/sysdeps/unix/sysv/linux/ifaddrs.c
index f62510811a..ef87b00423 100644
--- a/sysdeps/unix/sysv/linux/ifaddrs.c
+++ b/sysdeps/unix/sysv/linux/ifaddrs.c
@@ -150,7 +150,7 @@ __netlink_request (struct netlink_handle *h, int type)
   struct iovec iov = { buf, buf_size };

   if (__netlink_sendreq (h, type) < 0)
-    goto out_fail;
+    return -1;

   while (! done)
     {
@@ -168,13 +168,13 @@ __netlink_request (struct netlink_handle *h, int type)
       read_len = TEMP_FAILURE_RETRY (__recvmsg (h->fd, &msg, 0));
       __netlink_assert_response (h->fd, read_len);
       if (read_len < 0)
-       goto out_fail;
+       return -1;

       if (nladdr.nl_pid != 0)
        continue;

       if (__glibc_unlikely (msg.msg_flags & MSG_TRUNC))
-       goto out_fail;
+       return -1;

       size_t count = 0;
       size_t remaining_len = read_len;
@@ -200,7 +200,7 @@ __netlink_request (struct netlink_handle *h, int type)
                errno = EIO;
              else
                errno = -nlerr->error;
-             goto out_fail;
+             return -1;
            }
        }

@@ -212,7 +212,7 @@ __netlink_request (struct netlink_handle *h, int type)
       nlm_next = (struct netlink_res *) malloc (sizeof (struct netlink_res)
                                                + read_len);
       if (nlm_next == NULL)
-       goto out_fail;
+       return -1;
       nlm_next->next = NULL;
       nlm_next->nlh = memcpy (nlm_next + 1, buf, read_len);
       nlm_next->size = read_len;
@@ -225,9 +225,6 @@ __netlink_request (struct netlink_handle *h, int type)
     }

   return 0;
-
-out_fail:
-  return -1;
 }

LGTM with this change.

>  
>    struct iovec iov = { buf, buf_size };
>  
> @@ -229,13 +224,9 @@ __netlink_request (struct netlink_handle *h, int type)
>        h->end_ptr = nlm_next;
>      }
>  
> -  if (use_malloc)
> -    free (buf);
>    return 0;
>  
>  out_fail:
> -  if (use_malloc)
> -    free (buf);
>    return -1;
>  }
>  
> @@ -324,6 +315,8 @@ getifaddrs_internal (struct ifaddrs **ifap)
>    char *ifa_data_ptr;	/* Pointer to the unused part of memory for
>  				ifa_data.  */
>    int result = 0;
> +  struct scratch_buffer buf;
> +  scratch_buffer_init (&buf);
>  
>    *ifap = NULL;
>  
> @@ -425,7 +418,12 @@ getifaddrs_internal (struct ifaddrs **ifap)
>      }
>  
>    /* Table for mapping kernel index to entry in our list.  */
> -  map_newlink_data = alloca (newlink * sizeof (int));
> +  if (!scratch_buffer_set_array_size (&buf, 1, newlink * sizeof (int)))
> +    {
> +      result = -1;
> +      goto exit_free;
> +    }
> +  map_newlink_data = buf.data;
>    memset (map_newlink_data, '\xff', newlink * sizeof (int));
>  
>    ifa_data_ptr = (char *) &ifas[newlink + newaddr];
> @@ -820,6 +818,7 @@ getifaddrs_internal (struct ifaddrs **ifap)
>   exit_free:
>    __netlink_free_handle (&nh);
>    __netlink_close (&nh);
> +  scratch_buffer_free (&buf);
>  
>    return result;
>  }

Re: [PATCH v3] ifaddrs: Get rid of alloca

[Jeff Law]

On 5/30/23 09:25, Joe Simmons-Talbott via Libc-alpha wrote:
> Use scratch_buffer and malloc rather than alloca to avoid potential stack
> overflows.
> ---
> Changes to v2:
>    * Initialize the scratch_buffer earlier to avoid memory access errors
>      when calling scratch_buffer_free for failure cases.
> 
> Changes to v1:
>    * in __netlink_request use an 8kb buffer size and malloc rather than a
>      scratch_buffer.
> 
>      Suggested-by: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
[ ... ]
Just wanted to say thanks for doing this.  alloca has been a nightmare 
through the years from a security standpoint.

I advocated for its removal from glibc years ago based on the simple 
fact that the bugs exposed by the "bad guys" showed that even 
experienced developers are prone to get this stuff wrong.

Instead I wanted to have GCC prove particular allocations were safe to 
transform into alloca.  That never panned out, but I still think the 
compiler is the right place to exploit the performance improvements one 
gets from alloca vs malloc/free.

jeff

Re: [PATCH v3] ifaddrs: Get rid of alloca

[Florian Weimer]

* Jeff Law via Libc-alpha:

> Instead I wanted to have GCC prove particular allocations were safe to
> transform into alloca.  That never panned out, but I still think the
> compiler is the right place to exploit the performance improvements
> one gets from alloca vs malloc/free.

We got -fstack-clash-protection, which is I think is good enough from a
security point of view.

Thanks,
Florian

Re: [PATCH v3] ifaddrs: Get rid of alloca

[Jeff Law]

On 6/1/23 09:05, Florian Weimer wrote:
> * Jeff Law via Libc-alpha:
> 
>> Instead I wanted to have GCC prove particular allocations were safe to
>> transform into alloca.  That never panned out, but I still think the
>> compiler is the right place to exploit the performance improvements
>> one gets from alloca vs malloc/free.
> 
> We got -fstack-clash-protection, which is I think is good enough from a
> security point of view.
Yea...  Though I still want to dispose of explicitly using alloca :-)

Interestingly enough I'm likey to have to spin up stack-clash-protection 
within the next few months for RISC-V.   Though I may look to delegate 
it -- it'd be a great learning project for one of my compiler engineers.

jeff