[PATCH] stdmem: free: only call _gcry_secmem_free if needed

[Jussi Kivilinna]

Hello,

On 29.10.2018 15.37, Werner Koch wrote:
> Hi!
> 
> Thanks for this patch. I would even consider this for backporting.
> 
>> -  else if (!_gcry_secmem_free (p))
>> +
>> +  if (!_gcry_private_is_secure (freep) ||
>> +      !_gcry_secmem_free (freep))
> 
> Not related to the above change but in general I think it is important
> to add least add more comments to the code to warn about the assumptions
> we make.

Ok, I'll add some comments.

> 
> In particular when we allocate a new overflow pool in the secure memory
> we do this (in a locked state):
> 
> --8<---------------cut here---------------start------------->8---
>       /* Take care: in _gcry_private_is_secure we do not lock and thus
>        * we assume that the second assignment below is atomic.  */
>       pool->next = mainpool.next;
>       mainpool.next = pool;
> --8<---------------cut here---------------end--------------->8---
> 
> MAINPOOL is never changed and thus as long the last assignment is atomic
> we should always be able to call:
> 
> --8<---------------cut here---------------start------------->8---
> /* Return true if P points into the secure memory areas.  */
> int
> _gcry_private_is_secure (const void *p)
> {
>   pooldesc_t *pool;
> 
>   /* We do no lock here because once a pool is allocated it will not
>    * be removed anymore (except for gcry_secmem_term).  Further,
>    * adding a new pool to the list should be atomic.  */
>   for (pool = &mainpool; pool; pool = pool->next)
>     if (pool->okay && ptr_into_pool_p (pool, p))
>       return 1;
> 
>   return 0;
> }
> --8<---------------cut here---------------end--------------->8---
> 
> Do we need to think about memory visibility here?  Are there case where
> we pass a P which has been allocated in secure memory but the test does
> not catch it because the thread in the second function and the one in
> the first have a different view of the memory?  Bypassing locking or
> having no guaranteed memory barrier is a difficult thing.  Just asking.
> 

I do see one problem. The two stores when adding new overflow pool might
get re-ordered:
    1>   reg = mainpool.next;
    2>   pool->next = reg;
    3>   mainpool.next = pool;
to
    1>   reg = mainpool.next;
    3>   mainpool.next = pool;
    2>   pool->next = reg;

Now _gcry_private_is_secure might see load 3 but not 2, and exit loop
too early (pool->next == NULL, should see pool->next == old mainpool.next).

I think adding memory barrier before setting mainpool.next is enough to 
prevent this. That ensures that thread in _gcry_private_is_secure sees
new pool structure properly initialized.
    1>   reg = mainpool.next;
    2>   pool->next = reg;
         memory_barrier ();
    3>   mainpool.next = pool;

-Jussi