* [PATCH] Update tcache double-free check @ 2020-07-24 13:37 Eyal Itkin 2020-07-24 21:05 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Eyal Itkin @ 2020-07-24 13:37 UTC (permalink / raw) To: GNU C Library [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-07-24 13:37 [PATCH] Update tcache double-free check Eyal Itkin @ 2020-07-24 21:05 ` Carlos O'Donell 2020-07-25 10:39 ` Eyal Itkin 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2020-07-24 21:05 UTC (permalink / raw) To: Eyal Itkin, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-07-24 21:05 ` Carlos O'Donell @ 2020-07-25 10:39 ` Eyal Itkin 2020-07-25 21:07 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Eyal Itkin @ 2020-07-25 10:39 UTC (permalink / raw) To: Carlos O'Donell; +Cc: GNU C Library 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-07-25 10:39 ` Eyal Itkin @ 2020-07-25 21:07 ` Carlos O'Donell 2020-08-10 13:07 ` Eyal Itkin 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2020-07-25 21:07 UTC (permalink / raw) To: Eyal Itkin, Florian Weimer; +Cc: GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-07-25 21:07 ` Carlos O'Donell @ 2020-08-10 13:07 ` Eyal Itkin 2020-08-10 13:12 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Eyal Itkin @ 2020-08-10 13:07 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library [-- 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-08-10 13:07 ` Eyal Itkin @ 2020-08-10 13:12 ` Carlos O'Donell 2020-08-10 13:35 ` Eyal Itkin 2020-08-26 20:40 ` Carlos O'Donell 0 siblings, 2 replies; 15+ messages in thread From: Carlos O'Donell @ 2020-08-10 13:12 UTC (permalink / raw) To: Eyal Itkin; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-08-10 13:12 ` Carlos O'Donell @ 2020-08-10 13:35 ` Eyal Itkin 2020-08-10 13:44 ` Carlos O'Donell 2021-07-02 7:24 ` Siddhesh Poyarekar 2020-08-26 20:40 ` Carlos O'Donell 1 sibling, 2 replies; 15+ messages in thread From: Eyal Itkin @ 2020-08-10 13:35 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-08-10 13:35 ` Eyal Itkin @ 2020-08-10 13:44 ` Carlos O'Donell 2021-07-02 7:24 ` Siddhesh Poyarekar 1 sibling, 0 replies; 15+ messages in thread From: Carlos O'Donell @ 2020-08-10 13:44 UTC (permalink / raw) To: Eyal Itkin; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-08-10 13:35 ` Eyal Itkin 2020-08-10 13:44 ` Carlos O'Donell @ 2021-07-02 7:24 ` Siddhesh Poyarekar 2021-07-02 7:57 ` Eyal Itkin 1 sibling, 1 reply; 15+ messages in thread From: Siddhesh Poyarekar @ 2021-07-02 7:24 UTC (permalink / raw) To: Eyal Itkin, Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Update tcache double-free check 2021-07-02 7:24 ` Siddhesh Poyarekar @ 2021-07-02 7:57 ` Eyal Itkin 2021-07-02 8:45 ` Siddhesh Poyarekar 0 siblings, 1 reply; 15+ messages in thread From: Eyal Itkin @ 2021-07-02 7:57 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2021-07-02 7:57 ` Eyal Itkin @ 2021-07-02 8:45 ` Siddhesh Poyarekar 0 siblings, 0 replies; 15+ messages in thread From: Siddhesh Poyarekar @ 2021-07-02 8:45 UTC (permalink / raw) To: Eyal Itkin; +Cc: Carlos O'Donell, Florian Weimer, GNU C Library 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-08-10 13:12 ` Carlos O'Donell 2020-08-10 13:35 ` Eyal Itkin @ 2020-08-26 20:40 ` Carlos O'Donell 2020-10-03 9:04 ` Eyal Itkin 1 sibling, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2020-08-26 20:40 UTC (permalink / raw) To: Eyal Itkin; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-08-26 20:40 ` Carlos O'Donell @ 2020-10-03 9:04 ` Eyal Itkin 2020-10-04 19:41 ` Carlos O'Donell 0 siblings, 1 reply; 15+ messages in thread From: Eyal Itkin @ 2020-10-03 9:04 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-10-03 9:04 ` Eyal Itkin @ 2020-10-04 19:41 ` Carlos O'Donell 2020-10-14 13:44 ` Eyal Itkin 0 siblings, 1 reply; 15+ messages in thread From: Carlos O'Donell @ 2020-10-04 19:41 UTC (permalink / raw) To: Eyal Itkin; +Cc: Florian Weimer, GNU C Library 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Update tcache double-free check 2020-10-04 19:41 ` Carlos O'Donell @ 2020-10-14 13:44 ` Eyal Itkin 0 siblings, 0 replies; 15+ messages in thread From: Eyal Itkin @ 2020-10-14 13:44 UTC (permalink / raw) To: Carlos O'Donell; +Cc: Florian Weimer, GNU C Library 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. > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-07-02 8:46 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-24 13:37 [PATCH] Update tcache double-free check Eyal Itkin 2020-07-24 21:05 ` Carlos O'Donell 2020-07-25 10:39 ` Eyal Itkin 2020-07-25 21:07 ` Carlos O'Donell 2020-08-10 13:07 ` Eyal Itkin 2020-08-10 13:12 ` Carlos O'Donell 2020-08-10 13:35 ` Eyal Itkin 2020-08-10 13:44 ` Carlos O'Donell 2021-07-02 7:24 ` Siddhesh Poyarekar 2021-07-02 7:57 ` Eyal Itkin 2021-07-02 8:45 ` Siddhesh Poyarekar 2020-08-26 20:40 ` Carlos O'Donell 2020-10-03 9:04 ` Eyal Itkin 2020-10-04 19:41 ` Carlos O'Donell 2020-10-14 13:44 ` Eyal Itkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).