[PATCH] Do not leave random talloc magic in free()'ed memory

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Do not leave random talloc magic in free()'ed memory

Samba - samba-technical mailing list
G'Day,

I've been thinking about ways that our talloc magic protection might be
avoided and reading the magic from memory that has recently been
free()ed would be a good attack.

So this patch marks this memory with a fixed magic.  All valid use of
memory still uses the random magic.

This passed a full autobuild.

Please carefully review!  

On my re-look it might need to tweak talloc_chunk_from_ptr() a little
(when other flags could be set), but I would like other thoughts too!

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

talloc-magic-protection.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory

Samba - samba-technical mailing list
On Thu, 2017-12-21 at 20:13 +1300, Andrew Bartlett via samba-technical
wrote:

> G'Day,
>
> I've been thinking about ways that our talloc magic protection might be
> avoided and reading the magic from memory that has recently been
> free()ed would be a good attack.
>
> So this patch marks this memory with a fixed magic.  All valid use of
> memory still uses the random magic.
>
> This passed a full autobuild.
>
> Please carefully review!  
>
> On my re-look it might need to tweak talloc_chunk_from_ptr() a little
> (when other flags could be set), but I would like other thoughts too!
Attached is a revised set of patches, which removes the
talloc_abort_magic() branch as I can't see how it is usefully
triggered.

Clearly this needs very careful review.

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba




talloc-security.patch.txt (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Thu, 2017-12-21 at 20:13 +1300, Andrew Bartlett via samba-technical
wrote:

> G'Day,
>
> I've been thinking about ways that our talloc magic protection might be
> avoided and reading the magic from memory that has recently been
> free()ed would be a good attack.
>
> So this patch marks this memory with a fixed magic.  All valid use of
> memory still uses the random magic.
>
> This passed a full autobuild.
>
> Please carefully review!  
>
> On my re-look it might need to tweak talloc_chunk_from_ptr() a little
> (when other flags could be set), but I would like other thoughts too!
Attached is two patches, one which shows that the magic detection logic
was wrong, and the new no-disclosure stuff.

I think this is now ready.  Please review!

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba




talloc-magic.patch.txt (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message

Samba - samba-technical mailing list
On Fri, 2018-01-12 at 11:31 +1300, Andrew Bartlett wrote:

> On Thu, 2017-12-21 at 20:13 +1300, Andrew Bartlett via samba-technical
> wrote:
> > G'Day,
> >
> > I've been thinking about ways that our talloc magic protection might be
> > avoided and reading the magic from memory that has recently been
> > free()ed would be a good attack.
> >
> > So this patch marks this memory with a fixed magic.  All valid use of
> > memory still uses the random magic.
> >
> > This passed a full autobuild.
> >
> > Please carefully review!  
> >
> > On my re-look it might need to tweak talloc_chunk_from_ptr() a little
> > (when other flags could be set), but I would like other thoughts too!
>
> Attached is two patches, one which shows that the magic detection logic
> was wrong, and the new no-disclosure stuff.
>
> I think this is now ready.  Please review!
Gary has kindly reviewed these, I attach the reviewed patch.

Thanks,

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba




talloc-magic.patch.txt (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
Hi Andrew,

I saw these patches in your current autobuild.

I tried to pick them for 4.8.0 (including talloc-2.1.11),
but it failed locally with:

[2(1)/2 at 0s] samba4.local.talloc
talloc: access after free error - first free may be at
../lib/talloc/talloc.c:1978_talloc_realloc
Bad talloc magic value - access after free
UNEXPECTED(error): samba4.local.talloc.magic_free_protection(none)
REASON: Exception: Exception: Test was never started
UNEXPECTED(error): samba4.local.talloc.magic_protection(none)
(samba.subunit.RemotedTestCase)
REASON: was started but never finished!
UNEXPECTED(error): samba4.local.talloc.talloc(none)
(samba.subunit.RemotedTestCase)
REASON: was started but never finished!

I don't think we need this for 4.8.0rc1.

metze

Am 08.01.2018 um 05:38 schrieb Andrew Bartlett via samba-technical:

> On Thu, 2017-12-21 at 20:13 +1300, Andrew Bartlett via samba-technical
> wrote:
>> G'Day,
>>
>> I've been thinking about ways that our talloc magic protection might be
>> avoided and reading the magic from memory that has recently been
>> free()ed would be a good attack.
>>
>> So this patch marks this memory with a fixed magic.  All valid use of
>> memory still uses the random magic.
>>
>> This passed a full autobuild.
>>
>> Please carefully review!  
>>
>> On my re-look it might need to tweak talloc_chunk_from_ptr() a little
>> (when other flags could be set), but I would like other thoughts too!
>
> Attached is a revised set of patches, which removes the
> talloc_abort_magic() branch as I can't see how it is usefully
> triggered.
>
> Clearly this needs very careful review.
>
> Andrew Bartlett
>


signature.asc (853 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory

Samba - samba-technical mailing list
On Fri, 2018-01-12 at 08:21 +0100, Stefan Metzmacher wrote:

> Hi Andrew,
>
> I saw these patches in your current autobuild.
>
> I tried to pick them for 4.8.0 (including talloc-2.1.11),
> but it failed locally with:
>
> [2(1)/2 at 0s] samba4.local.talloc
> talloc: access after free error - first free may be at
> ../lib/talloc/talloc.c:1978_talloc_realloc
> Bad talloc magic value - access after free
> UNEXPECTED(error): samba4.local.talloc.magic_free_protection(none)
> REASON: Exception: Exception: Test was never started
> UNEXPECTED(error): samba4.local.talloc.magic_protection(none)
> (samba.subunit.RemotedTestCase)
> REASON: was started but never finished!
> UNEXPECTED(error): samba4.local.talloc.talloc(none)
> (samba.subunit.RemotedTestCase)
> REASON: was started but never finished!
>
> I don't think we need this for 4.8.0rc1.

Drat.  I tested those in the lib/talloc sub-build but not in the
included master build.  Probably just needs a success line, but I
agree, it is just a 'nice to have' and I have bugs filed in any case.

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory

Samba - samba-technical mailing list
On Fri, 2018-01-12 at 21:07 +1300, Andrew Bartlett wrote:

> On Fri, 2018-01-12 at 08:21 +0100, Stefan Metzmacher wrote:
> > Hi Andrew,
> >
> > I saw these patches in your current autobuild.
> >
> > I tried to pick them for 4.8.0 (including talloc-2.1.11),
> > but it failed locally with:
> >
> > [2(1)/2 at 0s] samba4.local.talloc
> > talloc: access after free error - first free may be at
> > ../lib/talloc/talloc.c:1978_talloc_realloc
> > Bad talloc magic value - access after free
> > UNEXPECTED(error): samba4.local.talloc.magic_free_protection(none)
> > REASON: Exception: Exception: Test was never started
> > UNEXPECTED(error): samba4.local.talloc.magic_protection(none)
> > (samba.subunit.RemotedTestCase)
> > REASON: was started but never finished!
> > UNEXPECTED(error): samba4.local.talloc.talloc(none)
> > (samba.subunit.RemotedTestCase)
> > REASON: was started but never finished!
> >
> > I don't think we need this for 4.8.0rc1.
>
> Drat.  I tested those in the lib/talloc sub-build but not in the
> included master build.  Probably just needs a success line, but I
> agree, it is just a 'nice to have' and I have bugs filed in any case.
For the curious, here is the fixup diff.

I'll do some trial autobuilds with my full set, I'm also doing some on
master.

Andrew Bartlett

--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

talloc-magic-test-fix.patch.txt (540 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, Jan 12, 2018 at 01:48:15PM +1300, Andrew Bartlett via samba-technical wrote:

> On Fri, 2018-01-12 at 11:31 +1300, Andrew Bartlett wrote:
> > On Thu, 2017-12-21 at 20:13 +1300, Andrew Bartlett via samba-technical
> > wrote:
> > > G'Day,
> > >
> > > I've been thinking about ways that our talloc magic protection might be
> > > avoided and reading the magic from memory that has recently been
> > > free()ed would be a good attack.
> > >
> > > So this patch marks this memory with a fixed magic.  All valid use of
> > > memory still uses the random magic.
> > >
> > > This passed a full autobuild.
> > >
> > > Please carefully review!  
> > >
> > > On my re-look it might need to tweak talloc_chunk_from_ptr() a little
> > > (when other flags could be set), but I would like other thoughts too!
> >
> > Attached is two patches, one which shows that the magic detection logic
> > was wrong, and the new no-disclosure stuff.
> >
> > I think this is now ready.  Please review!
>
> Gary has kindly reviewed these, I attach the reviewed patch.

Really nice work Andrew, thanks. Also RB+.

> Andrew Bartlett
>
> --
> Andrew Bartlett
> https://samba.org/~abartlet/
> Authentication Developer, Samba Team         https://samba.org
> Samba Development and Support, Catalyst IT  
> https://catalyst.net.nz/services/samba
>
>
>

> From 86ae186010af55bfc5c671b3d31f5a6cb76db52b Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Mon, 8 Jan 2018 17:29:19 +1300
> Subject: [PATCH 1/3] talloc: Remove talloc_abort_magic()
>
> The check required for talloc_abort_magic() prevents the 'access after free error'
> from being printed.
>
> It is also no longer possible to determine the difference between invalid memory
> and a talloc version mismatch as the magic is now random on many platforms.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13210
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> Reviewed-by: Gary Lockyer <[hidden email]>
> ---
>  lib/talloc/talloc.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index 7721fa4a9c6..3569ba75929 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -429,11 +429,6 @@ static void talloc_abort(const char *reason)
>   talloc_abort_fn(reason);
>  }
>  
> -static void talloc_abort_magic(unsigned magic)
> -{
> - talloc_abort("Bad talloc magic value - wrong talloc version used/mixed");
> -}
> -
>  static void talloc_abort_access_after_free(void)
>  {
>   talloc_abort("Bad talloc magic value - access after free");
> @@ -450,19 +445,14 @@ static inline struct talloc_chunk *talloc_chunk_from_ptr(const void *ptr)
>   const char *pp = (const char *)ptr;
>   struct talloc_chunk *tc = discard_const_p(struct talloc_chunk, pp - TC_HDR_SIZE);
>   if (unlikely((tc->flags & (TALLOC_FLAG_FREE | ~TALLOC_FLAG_MASK)) != talloc_magic)) {
> - if ((tc->flags & (~TALLOC_FLAG_MASK)) == talloc_magic) {
> - talloc_abort_magic(tc->flags & (~TALLOC_FLAG_MASK));
> - return NULL;
> - }
> -
> - if (tc->flags & TALLOC_FLAG_FREE) {
> - talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> - talloc_abort_access_after_free();
> - return NULL;
> - } else {
> + if ((tc->flags & (~TALLOC_FLAG_MASK)) != talloc_magic) {
>   talloc_abort_unknown_value();
>   return NULL;
>   }
> +
> + talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> + talloc_abort_access_after_free();
> + return NULL;
>   }
>   return tc;
>  }
> --
> 2.11.0
>
>
> From 709e6938ce7893f2d4a0664adb5e564dc25c5f5e Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Fri, 12 Jan 2018 11:17:09 +1300
> Subject: [PATCH 2/3] talloc: Add tests to require use-after-free to give the
>  correct talloc_abort() string
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13210
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> Reviewed-by: Gary Lockyer <[hidden email]>
> ---
>  lib/talloc/testsuite.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 68 insertions(+)
>
> diff --git a/lib/talloc/testsuite.c b/lib/talloc/testsuite.c
> index dfaeec1d1d9..92081b5fdd8 100644
> --- a/lib/talloc/testsuite.c
> +++ b/lib/talloc/testsuite.c
> @@ -2006,6 +2006,72 @@ static bool test_magic_protection(void)
>   return true;
>  }
>  
> +static void test_magic_free_protection_abort(const char *reason)
> +{
> + /* exit with errcode 42 to communicate successful test to the parent process */
> + if (strcmp(reason, "Bad talloc magic value - access after free") == 0) {
> + _exit(42);
> + }
> + /* not 42 */
> + _exit(404);
> +}
> +
> +static bool test_magic_free_protection(void)
> +{
> + void *pool = talloc_pool(NULL, 1024);
> + int *p1, *p2, *p3;
> + pid_t pid;
> + int exit_status;
> +
> + printf("test: magic_protection\n");
> + p1 = talloc(pool, int);
> + p2 = talloc(pool, int);
> +
> + /* To avoid complaints from the compiler assign values to the p1 & p2. */
> + *p1 = 6;
> + *p2 = 9;
> +
> + p3 = talloc_realloc(pool, p2, int, 2048);
> + torture_assert("pool realloc 2048",
> +       p3 != p2,
> +       "failed: pointer not changed");
> +
> + /*
> + * Now access the memory in the pool after the realloc().  It
> + * should be marked as free, so use of the old pointer should
> + * trigger the abort function
> + */
> + pid = fork();
> + if (pid == 0) {
> + talloc_set_abort_fn(test_magic_free_protection_abort);
> +
> + talloc_get_name(p2);
> +
> + /* Never reached. Make compilers happy */
> + return true;
> + }
> +
> + while (wait(&exit_status) != pid);
> +
> + if (!WIFEXITED(exit_status)) {
> + printf("Child exited through unexpected abnormal means\n");
> + return false;
> + }
> + if (WEXITSTATUS(exit_status) != 42) {
> + printf("Child exited with wrong exit status\n");
> + return false;
> + }
> + if (WIFSIGNALED(exit_status)) {
> + printf("Child recieved unexpected signal\n");
> + return false;
> + }
> +
> + talloc_free(pool);
> +
> + printf("success: magic_free_protection\n");
> + return true;
> +}
> +
>  static void test_reset(void)
>  {
>   talloc_set_log_fn(test_log_stdout);
> @@ -2092,6 +2158,8 @@ bool torture_local_talloc(struct torture_context *tctx)
>   ret &= test_autofree();
>   test_reset();
>   ret &= test_magic_protection();
> + test_reset();
> + ret &= test_magic_free_protection();
>  
>   test_reset();
>   talloc_disable_null_tracking();
> --
> 2.11.0
>
>
> From 659d3245494e955060e1b0d5b43c6e7faa90b1ee Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Mon, 8 Jan 2018 17:34:31 +1300
> Subject: [PATCH 3/3] talloc: Do not disclose the random talloc magic in
>  free()'ed memory
>
> This may help us avoid exploits via memory read attacks on Samba by ensuring that if the read
> is on an invalid chunk that the talloc magic disclosed there is not useful
> to create a valid chunk and so set a destructor.
>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=13211
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> Reviewed-by: Gary Lockyer <[hidden email]>
> ---
>  lib/talloc/talloc.c | 118 +++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 88 insertions(+), 30 deletions(-)
>
> diff --git a/lib/talloc/talloc.c b/lib/talloc/talloc.c
> index 3569ba75929..cd159ef89c2 100644
> --- a/lib/talloc/talloc.c
> +++ b/lib/talloc/talloc.c
> @@ -75,12 +75,13 @@
>  #define TALLOC_MAGIC_REFERENCE ((const char *)1)
>  
>  #define TALLOC_MAGIC_BASE 0xe814ec70
> -static unsigned int talloc_magic = (
> - ~TALLOC_FLAG_MASK & (
> - TALLOC_MAGIC_BASE +
> - (TALLOC_BUILD_VERSION_MAJOR << 24) +
> - (TALLOC_BUILD_VERSION_MINOR << 16) +
> - (TALLOC_BUILD_VERSION_RELEASE << 8)));
> +#define TALLOC_MAGIC_NON_RANDOM ( \
> + ~TALLOC_FLAG_MASK & ( \
> + TALLOC_MAGIC_BASE + \
> + (TALLOC_BUILD_VERSION_MAJOR << 24) + \
> + (TALLOC_BUILD_VERSION_MINOR << 16) + \
> + (TALLOC_BUILD_VERSION_RELEASE << 8)))
> +static unsigned int talloc_magic = TALLOC_MAGIC_NON_RANDOM;
>  
>  /* by default we abort when given a bad pointer (such as when talloc_free() is called
>     on a pointer that came from malloc() */
> @@ -332,6 +333,48 @@ _PUBLIC_ int talloc_test_get_magic(void)
>   return talloc_magic;
>  }
>  
> +static inline void _talloc_chunk_set_free(struct talloc_chunk *tc,
> +      const char *location)
> +{
> + /*
> + * Mark this memory as free, and also over-stamp the talloc
> + * magic with the old-style magic.
> + *
> + * Why?  This tries to avoid a memory read use-after-free from
> + * disclosing our talloc magic, which would then allow an
> + * attacker to prepare a valid header and so run a destructor.
> + *
> + */
> + tc->flags = TALLOC_MAGIC_NON_RANDOM | TALLOC_FLAG_FREE
> + | (tc->flags & TALLOC_FLAG_MASK);
> +
> + /* we mark the freed memory with where we called the free
> + * from. This means on a double free error we can report where
> + * the first free came from
> + */
> + if (location) {
> + tc->name = location;
> + }
> +}
> +
> +static inline void _talloc_chunk_set_not_free(struct talloc_chunk *tc)
> +{
> + /*
> + * Mark this memory as not free.
> + *
> + * Why? This is memory either in a pool (and so available for
> + * talloc's re-use or after the realloc().  We need to mark
> + * the memory as free() before any realloc() call as we can't
> + * write to the memory after that.
> + *
> + * We put back the normal magic instead of the 'not random'
> + * magic.
> + */
> +
> + tc->flags = talloc_magic |
> + ((tc->flags & TALLOC_FLAG_MASK) & ~TALLOC_FLAG_FREE);
> +}
> +
>  static void (*talloc_log_fn)(const char *message);
>  
>  _PUBLIC_ void talloc_set_log_fn(void (*log_fn)(const char *message))
> @@ -445,13 +488,14 @@ static inline struct talloc_chunk *talloc_chunk_from_ptr(const void *ptr)
>   const char *pp = (const char *)ptr;
>   struct talloc_chunk *tc = discard_const_p(struct talloc_chunk, pp - TC_HDR_SIZE);
>   if (unlikely((tc->flags & (TALLOC_FLAG_FREE | ~TALLOC_FLAG_MASK)) != talloc_magic)) {
> - if ((tc->flags & (~TALLOC_FLAG_MASK)) != talloc_magic) {
> - talloc_abort_unknown_value();
> + if ((tc->flags & (TALLOC_FLAG_FREE | ~TALLOC_FLAG_MASK))
> +    == (TALLOC_MAGIC_NON_RANDOM | TALLOC_FLAG_FREE)) {
> + talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> + talloc_abort_access_after_free();
>   return NULL;
>   }
>  
> - talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
> - talloc_abort_access_after_free();
> + talloc_abort_unknown_value();
>   return NULL;
>   }
>   return tc;
> @@ -937,13 +981,7 @@ static inline void _tc_free_poolmem(struct talloc_chunk *tc,
>   pool_tc = talloc_chunk_from_pool(pool);
>   next_tc = tc_next_chunk(tc);
>  
> - tc->flags |= TALLOC_FLAG_FREE;
> -
> - /* we mark the freed memory with where we called the free
> - * from. This means on a double free error we can report where
> - * the first free came from
> - */
> - tc->name = location;
> + _talloc_chunk_set_free(tc, location);
>  
>   TC_INVALIDATE_FULL_CHUNK(tc);
>  
> @@ -1093,13 +1131,7 @@ static inline int _tc_free_internal(struct talloc_chunk *tc,
>  
>   _tc_free_children_internal(tc, ptr, location);
>  
> - tc->flags |= TALLOC_FLAG_FREE;
> -
> - /* we mark the freed memory with where we called the free
> - * from. This means on a double free error we can report where
> - * the first free came from
> - */
> - tc->name = location;
> + _talloc_chunk_set_free(tc, location);
>  
>   if (tc->flags & TALLOC_FLAG_POOL) {
>   struct talloc_pool_hdr *pool;
> @@ -1796,8 +1828,22 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
>   }
>  #endif
>  
> - /* by resetting magic we catch users of the old memory */
> - tc->flags |= TALLOC_FLAG_FREE;
> + /*
> + * by resetting magic we catch users of the old memory
> + *
> + * We mark this memory as free, and also over-stamp the talloc
> + * magic with the old-style magic.
> + *
> + * Why?  This tries to avoid a memory read use-after-free from
> + * disclosing our talloc magic, which would then allow an
> + * attacker to prepare a valid header and so run a destructor.
> + *
> + * What else?  We have to re-stamp back a valid normal magic
> + * on this memory once realloc() is done, as it will have done
> + * a memcpy() into the new valid memory.  We can't do this in
> + * reverse as that would be a real use-after-free.
> + */
> + _talloc_chunk_set_free(tc, NULL);
>  
>  #if ALWAYS_REALLOC
>   if (pool_hdr) {
> @@ -1896,7 +1942,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
>  
>   if (new_chunk_size == old_chunk_size) {
>   TC_UNDEFINE_GROW_CHUNK(tc, size);
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + _talloc_chunk_set_not_free(tc);
>   tc->size = size;
>   return ptr;
>   }
> @@ -1911,7 +1957,7 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
>  
>   if (space_left >= space_needed) {
>   TC_UNDEFINE_GROW_CHUNK(tc, size);
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + _talloc_chunk_set_not_free(tc);
>   tc->size = size;
>   pool_hdr->end = tc_next_chunk(tc);
>   return ptr;
> @@ -1941,12 +1987,24 @@ _PUBLIC_ void *_talloc_realloc(const void *context, void *ptr, size_t size, cons
>  got_new_ptr:
>  #endif
>   if (unlikely(!new_ptr)) {
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + /*
> + * Ok, this is a strange spot.  We have to put back
> + * the old talloc_magic and any flags, except the
> + * TALLOC_FLAG_FREE as this was not free'ed by the
> + * realloc() call after all
> + */
> + _talloc_chunk_set_not_free(tc);
>   return NULL;
>   }
>  
> + /*
> + * tc is now the new value from realloc(), the old memory we
> + * can't access any more and was preemptively marked as
> + * TALLOC_FLAG_FREE before the call.  Now we mark it as not
> + * free again
> + */
>   tc = (struct talloc_chunk *)new_ptr;
> - tc->flags &= ~TALLOC_FLAG_FREE;
> + _talloc_chunk_set_not_free(tc);
>   if (malloced) {
>   tc->flags &= ~TALLOC_FLAG_POOLMEM;
>   }
> --
> 2.11.0
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Fri, 12 Jan 2018 13:48:15 +1300, Andrew Bartlett via samba-technical wrote:

> + talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);

If the talloc_chunk is potentially invalid here, then I'd prefer to
avoid logging any strings off it. Should we make this a "...%p\n", tc)
instead, or am I being overly paranoid?

Cheers, David

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Do not leave random talloc magic in free()'ed memory, fix abort message

Samba - samba-technical mailing list
On Sat, Jan 13, 2018 at 12:00:19AM +0100, David Disseldorp via samba-technical wrote:
> On Fri, 12 Jan 2018 13:48:15 +1300, Andrew Bartlett via samba-technical wrote:
>
> > + talloc_log("talloc: access after free error - first free may be at %s\n", tc->name);
>
> If the talloc_chunk is potentially invalid here, then I'd prefer to
> avoid logging any strings off it. Should we make this a "...%p\n", tc)
> instead, or am I being overly paranoid?

One can never be too paranoid with these things. Yep, I missed
that in the review. +1 for this change :-).