[PATCH] Update tcache double-free check

[PATCH] Update tcache double-free check

[Eyal Itkin]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

Hello,

As we discussed, I've attached here the patch for updating the
double-free check in the tcache. The patch passes all of malloc's
existing tests (including the double free tests), and it was tested to
work as expected both with and without entropy.

Once again, I apologize for sending the patch as an attachment instead
of inlined in the body of the mail itself (same Gmail issues as
before).

I am aware that there might be whitespace issues with the patch,
please feel free to fix them on your end if possible.
Thanks,
Eyal Itkin.

[-- Attachment #2: 0001-Update-tcache-double-free-check.patch --]
[-- Type: application/octet-stream, Size: 3038 bytes --]

From 32eee265a6574365246b9d89c68baed1e5aab53e Mon Sep 17 00:00:00 2001
From: Eyal Itkin <eyalit@checkpoint.com>
Date: Fri, 24 Jul 2020 16:09:33 +0300
Subject: [PATCH] Update tcache double-free check

Update the value used for the tcache entry->key when checking for
double free operations. Use a random value by default, and ~tcache as
a backup value if there isn't enough entropy / entropy isn't available.

Original key value was "tcache" which may lead to security issues in
code with use-after-free vulnerabilities ("House of Io" exploit). The
new key is no longer a valid pointer to a critical meta-data struct.
---
 malloc/malloc.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index ee87ddbbf9..37d6d62a6d 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -247,6 +247,9 @@
 /* For SINGLE_THREAD_P.  */
 #include <sysdep-cancel.h>
 
+/* For tcache double-free checks.  */
+#include <sys/random.h>
+
 /*
   Debugging:
 
@@ -2910,7 +2913,7 @@ typedef struct tcache_entry
 {
   struct tcache_entry *next;
   /* This field exists to detect double frees.  */
-  struct tcache_perthread_struct *key;
+  size_t key;
 } tcache_entry;
 
 /* There is one of these for each thread, which contains the
@@ -2926,6 +2929,7 @@ typedef struct tcache_perthread_struct
 
 static __thread bool tcache_shutting_down = false;
 static __thread tcache_perthread_struct *tcache = NULL;
+static __thread size_t tcache_key = 0;
 
 /* Caller must ensure that we know tc_idx is valid and there's room
    for more chunks.  */
@@ -2936,7 +2940,7 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
 
   /* Mark this chunk as "in the tcache" so the test in _int_free will
      detect a double free.  */
-  e->key = tcache;
+  e->key = tcache_key;
 
   e->next = PROTECT_PTR (&e->next, tcache->entries[tc_idx]);
   tcache->entries[tc_idx] = e;
@@ -2953,7 +2957,7 @@ tcache_get (size_t tc_idx)
     malloc_printerr ("malloc(): unaligned tcache chunk detected");
   tcache->entries[tc_idx] = REVEAL_PTR (e->next);
   --(tcache->counts[tc_idx]);
-  e->key = NULL;
+  e->key = 0;
   return (void *) e;
 }
 
@@ -3019,6 +3023,12 @@ tcache_init(void)
     {
       tcache = (tcache_perthread_struct *) victim;
       memset (tcache, 0, sizeof (tcache_perthread_struct));
+
+      /* Attempt to get a random key for the double-free checks.  */
+      int res = getrandom (&tcache_key, sizeof(tcache_key), GRND_NONBLOCK);
+      /* If failed, use the agreed alternative: ~tcache.  */
+      if (res != sizeof(tcache_key))
+        tcache_key = ~((size_t) tcache);
     }
 
 }
@@ -4218,7 +4228,7 @@ _int_free (mstate av, mchunkptr p, int have_lock)
 	   trust it (it also matches random payload data at a 1 in
 	   2^<size_t> chance), so verify it's not an unlikely
 	   coincidence before aborting.  */
-	if (__glibc_unlikely (e->key == tcache))
+	if (__glibc_unlikely (e->key == tcache_key))
 	  {
 	    tcache_entry *tmp;
 	    LIBC_PROBE (memory_tcache_double_free, 2, e, tc_idx);
-- 
2.17.1

Re: [PATCH] Update tcache double-free check

[Carlos O'Donell]

On 7/24/20 9:37 AM, Eyal Itkin via Libc-alpha wrote:
> Hello,
> 
> As we discussed, I've attached here the patch for updating the
> double-free check in the tcache. The patch passes all of malloc's
> existing tests (including the double free tests), and it was tested to
> work as expected both with and without entropy.
> 
> Once again, I apologize for sending the patch as an attachment instead
> of inlined in the body of the mail itself (same Gmail issues as
> before).
> 
> I am aware that there might be whitespace issues with the patch,
> please feel free to fix them on your end if possible.

Do we have any concerns having all threads call getrandom()?

If even *one* thread succeeded at calling getrandom() could we
use that to make ~tcache more random?

There is certainly a tradeoff here between the number of calls
to getrandom() and the values we use in the key. I haven't
measured the numbers to decide on this yet.

Do your changes show up in benchtests/bench-malloc-{simple,thread}.c?

-- 
Cheers,
Carlos.

Re: [PATCH] Update tcache double-free check

[Eyal Itkin]

I've gone over the compiled binaries before/after the patch, and the
change on my x64 binary is as follows: no change to tcache_get() or
tcache_put(), and an addition of 2 asm instructions to _int_free(). If
we would have only used ~tcache this would have been reduced to an
addition of a single asm instruction in _int_free().

The patch's impact on benchtests/bench-malloc-(simple,thread) is
negligible, and is less than the natural noise I get on my GCP
instance between two consecutive benchmark executions.

Since the call to getrandom() is only on tcache_init(), hence once per
thread, I'm fine with it being called for all threads, but it is
really for you to decide.

There is a possible fix to make sure that the entropy we get from
getrandom() will be accumulated, thus helping all future threads that
might need it. We won't be able to update the keys of previous threads
(if they lacked entropy), but we can enrich future threads with the
entropy even if they failed to use getrandom(). This solution will
also enable us to use all of the received entropy from getrandom()
even when we get less than the wanted amount of bytes.

The solution for such an entropy accumulation is quite simple:

static __thread size_t tcache_key = 0;
static size_t tcache_entropy = 0;

void tcache_init()
{
      ...
      /* Attempt to get a random key for the double-free checks.  */
      size_t local_entropy = 0;
      getrandom (&local_entropy, sizeof(local_entropy), GRND_NONBLOCK);
      /* always accumulate it with the past seen entropy */
      tcache_entropy ^= local_entropy;
      /* always use the basic alternative of ~tcache in case we didn't
have any entropy.  */
      tcache_key = tcache_entropy ^ ~((size_t) tcache);
}

As can be seen, in case that all calls to getrandom() failed, and we
received nothing from it, we would still use ~tcache, just like
before. In addition, this use of "~tcache" also adds the entropy from
ASLR to the tcache_key, so we don't lose anything from using it.

Please advise me of the best way to proceed.

On Sat, Jul 25, 2020 at 12:05 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 7/24/20 9:37 AM, Eyal Itkin via Libc-alpha wrote:
> > Hello,
> >
> > As we discussed, I've attached here the patch for updating the
> > double-free check in the tcache. The patch passes all of malloc's
> > existing tests (including the double free tests), and it was tested to
> > work as expected both with and without entropy.
> >
> > Once again, I apologize for sending the patch as an attachment instead
> > of inlined in the body of the mail itself (same Gmail issues as
> > before).
> >
> > I am aware that there might be whitespace issues with the patch,
> > please feel free to fix them on your end if possible.
>
> Do we have any concerns having all threads call getrandom()?
>
> If even *one* thread succeeded at calling getrandom() could we
> use that to make ~tcache more random?
>
> There is certainly a tradeoff here between the number of calls
> to getrandom() and the values we use in the key. I haven't
> measured the numbers to decide on this yet.
>
> Do your changes show up in benchtests/bench-malloc-{simple,thread}.c?
>
> --
> Cheers,
> Carlos.
>

Re: [PATCH] Update tcache double-free check

[Carlos O'Donell]

On 7/25/20 6:39 AM, Eyal Itkin wrote:
> I've gone over the compiled binaries before/after the patch, and the
> change on my x64 binary is as follows: no change to tcache_get() or
> tcache_put(), and an addition of 2 asm instructions to _int_free(). If
> we would have only used ~tcache this would have been reduced to an
> addition of a single asm instruction in _int_free().
> 
> The patch's impact on benchtests/bench-malloc-(simple,thread) is
> negligible, and is less than the natural noise I get on my GCP
> instance between two consecutive benchmark executions.
> 
> Since the call to getrandom() is only on tcache_init(), hence once per
> thread, I'm fine with it being called for all threads, but it is
> really for you to decide.
> 
> There is a possible fix to make sure that the entropy we get from
> getrandom() will be accumulated, thus helping all future threads that
> might need it. We won't be able to update the keys of previous threads
> (if they lacked entropy), but we can enrich future threads with the
> entropy even if they failed to use getrandom(). This solution will
> also enable us to use all of the received entropy from getrandom()
> even when we get less than the wanted amount of bytes.
> 
> The solution for such an entropy accumulation is quite simple:
> 
> static __thread size_t tcache_key = 0;
> static size_t tcache_entropy = 0;
> 
> void tcache_init()
> {
>       ...
>       /* Attempt to get a random key for the double-free checks.  */
>       size_t local_entropy = 0;
>       getrandom (&local_entropy, sizeof(local_entropy), GRND_NONBLOCK);
>       /* always accumulate it with the past seen entropy */
>       tcache_entropy ^= local_entropy;
>       /* always use the basic alternative of ~tcache in case we didn't
> have any entropy.  */
>       tcache_key = tcache_entropy ^ ~((size_t) tcache);
> }
> 
> As can be seen, in case that all calls to getrandom() failed, and we
> received nothing from it, we would still use ~tcache, just like
> before. In addition, this use of "~tcache" also adds the entropy from
> ASLR to the tcache_key, so we don't lose anything from using it.
> 
> Please advise me of the best way to proceed.

I like the accumulation of entropy. I'm a little worried about thread
startup and shutdown costs, but I haven't measured that yet. I'm also
busy getting the release out the door (I'm the release manager).
If you have thread startup/shutdown costs that would be interesting
for me to see as a reviewer.

I'd like to see Florian's opinion on this.

I fully expect that if you flesh this out you'll have to handle the
getrandom failure case, and the atomics required to avoid the data
race with multiple threads and global state. However, hold off on
that until we get a big more consensus if we really want to go to
this level of complexity. It would be good to keep the entropy we
have in the event the system is under some kind of entropy starvation
at a later point (I've seen at least one of these for TCP sequence
numbers, read and drop from /dev/random etc.)

-- 
Cheers,
Carlos.

Re: [PATCH] Update tcache double-free check

[Eyal Itkin]

[-- Attachment #1: Type: text/plain, Size: 3810 bytes --]

Updated the patch to perform an atomic update operation on the global
entropy state, so to avoid races when multiple threads are initialized
simultaneously. The patch now accumulates entropy between threads,
while still using the backup case of ~tcache to take care of cases in
which no entropy was yet to be available.

As Carlos mentioned earlier, I guess you will want to discuss this
patch before integrating it. Also, feel free to update the patch if
needed in case I missed some whitespace / long line coding style.

Thanks again,
Eyal


On Sun, Jul 26, 2020 at 12:07 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 7/25/20 6:39 AM, Eyal Itkin wrote:
> > I've gone over the compiled binaries before/after the patch, and the
> > change on my x64 binary is as follows: no change to tcache_get() or
> > tcache_put(), and an addition of 2 asm instructions to _int_free(). If
> > we would have only used ~tcache this would have been reduced to an
> > addition of a single asm instruction in _int_free().
> >
> > The patch's impact on benchtests/bench-malloc-(simple,thread) is
> > negligible, and is less than the natural noise I get on my GCP
> > instance between two consecutive benchmark executions.
> >
> > Since the call to getrandom() is only on tcache_init(), hence once per
> > thread, I'm fine with it being called for all threads, but it is
> > really for you to decide.
> >
> > There is a possible fix to make sure that the entropy we get from
> > getrandom() will be accumulated, thus helping all future threads that
> > might need it. We won't be able to update the keys of previous threads
> > (if they lacked entropy), but we can enrich future threads with the
> > entropy even if they failed to use getrandom(). This solution will
> > also enable us to use all of the received entropy from getrandom()
> > even when we get less than the wanted amount of bytes.
> >
> > The solution for such an entropy accumulation is quite simple:
> >
> > static __thread size_t tcache_key = 0;
> > static size_t tcache_entropy = 0;
> >
> > void tcache_init()
> > {
> >       ...
> >       /* Attempt to get a random key for the double-free checks.  */
> >       size_t local_entropy = 0;
> >       getrandom (&local_entropy, sizeof(local_entropy), GRND_NONBLOCK);
> >       /* always accumulate it with the past seen entropy */
> >       tcache_entropy ^= local_entropy;
> >       /* always use the basic alternative of ~tcache in case we didn't
> > have any entropy.  */
> >       tcache_key = tcache_entropy ^ ~((size_t) tcache);
> > }
> >
> > As can be seen, in case that all calls to getrandom() failed, and we
> > received nothing from it, we would still use ~tcache, just like
> > before. In addition, this use of "~tcache" also adds the entropy from
> > ASLR to the tcache_key, so we don't lose anything from using it.
> >
> > Please advise me of the best way to proceed.
>
> I like the accumulation of entropy. I'm a little worried about thread
> startup and shutdown costs, but I haven't measured that yet. I'm also
> busy getting the release out the door (I'm the release manager).
> If you have thread startup/shutdown costs that would be interesting
> for me to see as a reviewer.
>
> I'd like to see Florian's opinion on this.
>
> I fully expect that if you flesh this out you'll have to handle the
> getrandom failure case, and the atomics required to avoid the data
> race with multiple threads and global state. However, hold off on
> that until we get a big more consensus if we really want to go to
> this level of complexity. It would be good to keep the entropy we
> have in the event the system is under some kind of entropy starvation
> at a later point (I've seen at least one of these for TCP sequence
> numbers, read and drop from /dev/random etc.)
>
> --
> Cheers,
> Carlos.
>

[-- Attachment #2: 0001-Update-tcache-double-free-check.patch --]
[-- Type: application/octet-stream, Size: 3575 bytes --]

From baf66196c486f6ab68be340fdbffd0e0df784e77 Mon Sep 17 00:00:00 2001
From: Eyal Itkin <eyalit@checkpoint.com>
Date: Mon, 10 Aug 2020 15:58:08 +0300
Subject: [PATCH] Update tcache double-free check

Update the value used for the tcache entry->key when checking for
double free operations. Use a random value by default, and accumulate
the gathered entropy across time and across threads. In addition, use
~tcache as a backup value incase there isn't enough entropy / entropy
isn't available.

Original key value was "tcache" which may lead to security issues in
code with use-after-free vulnerabilities ("House of Io" exploit). The
new key is no longer a valid pointer to a critical meta-data struct.
---
 malloc/malloc.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index ee87ddbbf9..4e9a5e17ef 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -247,6 +247,9 @@
 /* For SINGLE_THREAD_P.  */
 #include <sysdep-cancel.h>
 
+/* For tcache double-free checks.  */
+#include <sys/random.h>
+
 /*
   Debugging:
 
@@ -2910,7 +2913,7 @@ typedef struct tcache_entry
 {
   struct tcache_entry *next;
   /* This field exists to detect double frees.  */
-  struct tcache_perthread_struct *key;
+  size_t key;
 } tcache_entry;
 
 /* There is one of these for each thread, which contains the
@@ -2926,6 +2929,8 @@ typedef struct tcache_perthread_struct
 
 static __thread bool tcache_shutting_down = false;
 static __thread tcache_perthread_struct *tcache = NULL;
+static __thread size_t tcache_key = 0;
+static size_t tcache_entropy = 0;
 
 /* Caller must ensure that we know tc_idx is valid and there's room
    for more chunks.  */
@@ -2936,7 +2941,7 @@ tcache_put (mchunkptr chunk, size_t tc_idx)
 
   /* Mark this chunk as "in the tcache" so the test in _int_free will
      detect a double free.  */
-  e->key = tcache;
+  e->key = tcache_key;
 
   e->next = PROTECT_PTR (&e->next, tcache->entries[tc_idx]);
   tcache->entries[tc_idx] = e;
@@ -2953,7 +2958,7 @@ tcache_get (size_t tc_idx)
     malloc_printerr ("malloc(): unaligned tcache chunk detected");
   tcache->entries[tc_idx] = REVEAL_PTR (e->next);
   --(tcache->counts[tc_idx]);
-  e->key = NULL;
+  e->key = 0;
   return (void *) e;
 }
 
@@ -3019,8 +3024,22 @@ tcache_init(void)
     {
       tcache = (tcache_perthread_struct *) victim;
       memset (tcache, 0, sizeof (tcache_perthread_struct));
-    }
 
+      /* Attempt to get a random key for the double-free checks.  */
+      size_t local_entropy = 0, updated_entropy, entropy, old;
+      getrandom (&local_entropy, sizeof(local_entropy), GRND_NONBLOCK);
+      /* Always accumulate it with past seen entropy.  */
+      old = tcache_entropy;
+      do
+        {
+          entropy = old;
+          updated_entropy = local_entropy ^ entropy;
+        }
+      while ((old = catomic_compare_and_exchange_val_acq (&tcache_entropy,
+        updated_entropy, entropy)) != entropy);
+      /* Always use the default agreed alternative: ~tcache.  */
+      tcache_key = updated_entropy ^ ~((size_t) tcache);
+    }
 }
 
 # define MAYBE_INIT_TCACHE() \
@@ -4218,7 +4237,7 @@ _int_free (mstate av, mchunkptr p, int have_lock)
 	   trust it (it also matches random payload data at a 1 in
 	   2^<size_t> chance), so verify it's not an unlikely
 	   coincidence before aborting.  */
-	if (__glibc_unlikely (e->key == tcache))
+	if (__glibc_unlikely (e->key == tcache_key))
 	  {
 	    tcache_entry *tmp;
 	    LIBC_PROBE (memory_tcache_double_free, 2, e, tc_idx);
-- 
2.17.1

Re: [PATCH] Update tcache double-free check

[Carlos O'Donell]

On 8/10/20 9:07 AM, Eyal Itkin wrote:
> Updated the patch to perform an atomic update operation on the global
> entropy state, so to avoid races when multiple threads are initialized
> simultaneously. The patch now accumulates entropy between threads,
> while still using the backup case of ~tcache to take care of cases in
> which no entropy was yet to be available.
> 
> As Carlos mentioned earlier, I guess you will want to discuss this
> patch before integrating it. Also, feel free to update the patch if
> needed in case I missed some whitespace / long line coding style.

Thank you for putting this together. I need to spend some time thinking
more deeply on this and considering where the right balance might lie
between a per-process value and a per-thread value. Particularly with
respect to the tradeoff between maintaining the code and security.

Do you have any strong opinions on the use of a per-thread vs. per-process
value?

This patch is third on my queue.

My queue is currently:
- NSS configuration reloading (DJ Delorie)
- DSO sorting DFS (Chung-Lin Tang)
- Tcache double-free check (Eyal Itkin)

-- 
Cheers,
Carlos.

Re: [PATCH] Update tcache double-free check

[Eyal Itkin]

The design of this feature started by stating that "any arbitrary
value" will suffice. Which will indeed be enough when protecting
against a double free vulnerability assuming the attacker has no
control over the content of the memory that is going to be free()ed
twice.

However, if we look at a proper threat model, we have the following:
1. Attacker that can't control the content of the memory to be free()ed
2. Attacker that has full/partial control over the content of the
memory to be free()ed

When examining the double free check, we can see that a double free
operation will be caught if the EXACT key will match. Which basically
means that if the attacker can modify even a single bit in the stored
key, the check will be bypassed. Even, without the attacker knowing
what was the original key.

This basically means that the only advantage of using a random value
is to avoid using a constant arbitrary value that might collide
(repeatedly) with specific values of the program, and will demand
slower checks in the tcache so as to rule out the possibility of a
double free. In that sense, double-free across threads will never be
caught, as the thread will still check it's own tcache struct and will
never find the other thread's buffer inside their own struct.

Usually, random values make a defense stronger because the attacker
has to guess each and every one of the bits. However, after performing
the analysis, it seems like we have the opposite case here. If the
attacker can modify even a single bit, a double-free operation will be
possible, as the check will be bypassed.

Due to that, I recommend using a random value per process according to
the original scheme of getrandom() and a backup of "~tcache". Keeping
in mind that the ASLR bits will supply some random, and that ~tcache
will never be a value we expect to see in memory and won't ever be a
mapped memory address that could be used by a use-after-free
vulnerability as the original key ("tcache") was exploited. I don't
see any value in a per-thread random key, as again, the random value
will only help us avoid collisions with values from the program's
memory.

The overall scheme for accumulating random between threads might be
useful in the future, for a cookie-style security check, however I
convinced myself that it won't be needed in this case.

Once a decision will be made, please notify me, and I'll update the
patch accordingly.


On Mon, Aug 10, 2020 at 4:12 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 8/10/20 9:07 AM, Eyal Itkin wrote:
> > Updated the patch to perform an atomic update operation on the global
> > entropy state, so to avoid races when multiple threads are initialized
> > simultaneously. The patch now accumulates entropy between threads,
> > while still using the backup case of ~tcache to take care of cases in
> > which no entropy was yet to be available.
> >
> > As Carlos mentioned earlier, I guess you will want to discuss this
> > patch before integrating it. Also, feel free to update the patch if
> > needed in case I missed some whitespace / long line coding style.
>
> Thank you for putting this together. I need to spend some time thinking
> more deeply on this and considering where the right balance might lie
> between a per-process value and a per-thread value. Particularly with
> respect to the tradeoff between maintaining the code and security.
>
> Do you have any strong opinions on the use of a per-thread vs. per-process
> value?
>
> This patch is third on my queue.
>
> My queue is currently:
> - NSS configuration reloading (DJ Delorie)
> - DSO sorting DFS (Chung-Lin Tang)
> - Tcache double-free check (Eyal Itkin)
>
> --
> Cheers,
> Carlos.
>

Re: [PATCH] Update tcache double-free check

[Carlos O'Donell]

On 8/10/20 9:35 AM, Eyal Itkin wrote:
> The overall scheme for accumulating random between threads might be
> useful in the future, for a cookie-style security check, however I
> convinced myself that it won't be needed in this case.

Your analysis seems sensible to me. In the case of a cookie-style check
I think we can and *should* do this with some of the chunk metadata.
This is something Florian proposed a couple of years ago, but the
difficulty is that it's straight on the hot path, so you have to try
reorder the operations to get as-good performance as before. Given that
you're already touching the cacheline with the metadata there is a lot
that you can hide e.g. xor of the size with a cookie.

You're right though the code you've written I think will be useful to
others.

I still need to review the various versions we have and get consensus.

-- 
Cheers,
Carlos.

Re: [PATCH] Update tcache double-free check

[Siddhesh Poyarekar]

Hello Eyal,

Carlos asked me to take a look at this because we think it's important 
to close this hole if we can.  Sorry we took long to get back to this.

On 8/10/20 7:05 PM, Eyal Itkin via Libc-alpha wrote:
> Due to that, I recommend using a random value per process according to
> the original scheme of getrandom() and a backup of "~tcache". Keeping
> in mind that the ASLR bits will supply some random, and that ~tcache
> will never be a value we expect to see in memory and won't ever be a
> mapped memory address that could be used by a use-after-free
> vulnerability as the original key ("tcache") was exploited. I don't
> see any value in a per-thread random key, as again, the random value
> will only help us avoid collisions with values from the program's
> memory.

Agreed, a per-process key is sufficient.  However your latest patch 
appears to have a per-thread key; I assume you intend to post an update 
patch based on feedback?

Given that our only goals are (1) put in some random value to avoid 
collisions and (2) not leak the address of an internal structure in the 
process, could this just be a single process-wide variable that is 
initialized at startup?

What this implies is that tcache_key (a static variable and not 
__thread) is initialized in ptmalloc_init instead of tcache_init and the 
same value is used in all threads.  ptmalloc_init is guaranteed to run 
in a single-threaded context as malloc gets called in the process of 
thread creation before it is spawned and hence should not need any 
synchronization.

Finally for randomness of the tcache_key, it might make sense to use 
getrandom first (as the patch already does) and then on failure, fall 
back to a munging of ~tcache and random_bits() (from 
include/random-bits.h) so that we don't leak addresses even on fallback. 
  However I'm not convinced that getrandom() is strictly necessary given 
our goals; I reckon it's just harmless given that it's a one time cost.

Siddhesh

PS: I'll make sure you're cc'd on this conversation, so you don't have 
to be registered to the mailing list to stay on track with this 
conversation.

[PATCH] Update tcache double-free check

[Eyal Itkin]

Hi,

Nice to see that this topic is still alive. As I said earlier,
although my initial patch was per-thread, my later analysis convinced
me that a per process solution will be a better idea.

As for the benchmarking, the cost per-thread was negligible, so I
don't see any potential risk with using the same solution (getrandom
and all) just one time per process.

Sadly, I suggest you will modify my original patch / recreate a
similar solution, as I can no longer commit new code to FSF. In the
time passed the approval of my original employer has expired (approval
was for a single year) and I also switched work place and will have to
undergo the entire legal process yet again.

Given the maturity of the current draft, I suggest you will complete
this feature based on my contribution (contribution that was made when
it was still allowed). Without an additional similar feature in the
near future, I don't see the benefit in troubling a VP for signing
again the legal docs.

Happy to see that this feature was not abandoned.

Good luck to your all, and thanks for your enthusiasm for improving
the security of such an important library.
Eyal Itkin.

Re: [PATCH] Update tcache double-free check

[Siddhesh Poyarekar]

On 7/2/21 1:27 PM, Eyal Itkin wrote:
> Nice to see that this topic is still alive. As I said earlier,
> although my initial patch was per-thread, my later analysis convinced
> me that a per process solution will be a better idea.
> 
> As for the benchmarking, the cost per-thread was negligible, so I
> don't see any potential risk with using the same solution (getrandom
> and all) just one time per process.

OK that's great, thanks for confirming.

> Sadly, I suggest you will modify my original patch / recreate a
> similar solution, as I can no longer commit new code to FSF. In the
> time passed the approval of my original employer has expired (approval
> was for a single year) and I also switched work place and will have to
> undergo the entire legal process yet again.

I understand.

> Given the maturity of the current draft, I suggest you will complete
> this feature based on my contribution (contribution that was made when
> it was still allowed). Without an additional similar feature in the
> near future, I don't see the benefit in troubling a VP for signing
> again the legal docs.
> 
> Happy to see that this feature was not abandoned.
> 
> Good luck to your all, and thanks for your enthusiasm for improving
> the security of such an important library.

No worries and thank you for sharing the idea and following up on it. 
I'll post a draft soon.

Thanks,
Siddhesh

Re: [PATCH] Update tcache double-free check

[Carlos O'Donell]

On 8/10/20 9:12 AM, Carlos O'Donell wrote:
> On 8/10/20 9:07 AM, Eyal Itkin wrote:
>> Updated the patch to perform an atomic update operation on the global
>> entropy state, so to avoid races when multiple threads are initialized
>> simultaneously. The patch now accumulates entropy between threads,
>> while still using the backup case of ~tcache to take care of cases in
>> which no entropy was yet to be available.
>>
>> As Carlos mentioned earlier, I guess you will want to discuss this
>> patch before integrating it. Also, feel free to update the patch if
>> needed in case I missed some whitespace / long line coding style.
> 
> Thank you for putting this together. I need to spend some time thinking
> more deeply on this and considering where the right balance might lie
> between a per-process value and a per-thread value. Particularly with
> respect to the tradeoff between maintaining the code and security.
> 
> Do you have any strong opinions on the use of a per-thread vs. per-process
> value?
> 
> This patch is third on my queue.
> 
> My queue is currently:
> - NSS configuration reloading (DJ Delorie)
> - DSO sorting DFS (Chung-Lin Tang)
> - Tcache double-free check (Eyal Itkin)
> 

I got through the NSS configuration patches.

I'm working again on DSO sorting.

LPC 2020 is interrupting this week.

Thanks for your patience.

-- 
Cheers,
Carlos.

Re: [PATCH] Update tcache double-free check

[Eyal Itkin]

Ping. Is there any update on this subject?


On Wed, Aug 26, 2020 at 11:40 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 8/10/20 9:12 AM, Carlos O'Donell wrote:
> > On 8/10/20 9:07 AM, Eyal Itkin wrote:
> >> Updated the patch to perform an atomic update operation on the global
> >> entropy state, so to avoid races when multiple threads are initialized
> >> simultaneously. The patch now accumulates entropy between threads,
> >> while still using the backup case of ~tcache to take care of cases in
> >> which no entropy was yet to be available.
> >>
> >> As Carlos mentioned earlier, I guess you will want to discuss this
> >> patch before integrating it. Also, feel free to update the patch if
> >> needed in case I missed some whitespace / long line coding style.
> >
> > Thank you for putting this together. I need to spend some time thinking
> > more deeply on this and considering where the right balance might lie
> > between a per-process value and a per-thread value. Particularly with
> > respect to the tradeoff between maintaining the code and security.
> >
> > Do you have any strong opinions on the use of a per-thread vs. per-process
> > value?
> >
> > This patch is third on my queue.
> >
> > My queue is currently:
> > - NSS configuration reloading (DJ Delorie)
> > - DSO sorting DFS (Chung-Lin Tang)
> > - Tcache double-free check (Eyal Itkin)
> >
>
> I got through the NSS configuration patches.
>
> I'm working again on DSO sorting.
>
> LPC 2020 is interrupting this week.
>
> Thanks for your patience.
>
> --
> Cheers,
> Carlos.
>

Re: [PATCH] Update tcache double-free check

[Carlos O'Donell]

On 10/3/20 5:04 AM, Eyal Itkin wrote:
> Ping. Is there any update on this subject?

Not yet.

My queue is:
- DSO sorting patches.
- Tcache double-free
- nsswitch (again)

I've been blocked by other deliverables.

I've reviewed some other patches, but they are
easier to reason about and review ;-)

-- 
Cheers,
Carlos.

Re: [PATCH] Update tcache double-free check

[Eyal Itkin]

Hi, the amount of emails I receive each day from this mailing list is
really making it hard for me to keep track of my personal / other work
related emails (not even mentioning a "Zero inbox" policy). Having it
for more than 2 months now while the patch is being examined is a bit
too much.

I'm unsubscribing myself from this list, and when there will be any
update about the proposed patch, please let me know via the ongoing
thread (that contains my email), or by creating a new one that I'll be
added to it explicitly.

Thanks,
Eyal.


On Sun, Oct 4, 2020 at 10:41 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 10/3/20 5:04 AM, Eyal Itkin wrote:
> > Ping. Is there any update on this subject?
>
> Not yet.
>
> My queue is:
> - DSO sorting patches.
> - Tcache double-free
> - nsswitch (again)
>
> I've been blocked by other deliverables.
>
> I've reviewed some other patches, but they are
> easier to reason about and review ;-)
>
> --
> Cheers,
> Carlos.
>