[PATCH] Linux: Fixes for getrandom fork handling

[Adhemerval Zanella Netto]

On 16/01/25 15:18, Florian Weimer wrote:
> Careful updates of grnd_alloc.len are required to ensure that
> after fork, grnd_alloc.states does not contain entries that
> are also encountered by __getrandom_reset_state in TCBs.
> For the same reason, it is necessary to overwrite the TCB state
> pointer with NULL before updating grnd_alloc.states in
> __getrandom_vdso_release.
> 
> Before this change, different TCBs could share the same getrandom
> state after multi-threaded fork.  This would be a critical security
> bug (predictable randomness) if not caught during development.
> 
> The additional check in stdlib/tst-arc4random-thread makes it more
> likely that the test fails due to the bugs mentioned above.
> 
> Both __getrandom_reset_state and __getrandom_vdso_release could
> put reserved NULL pointers into the states array.  This is also
> fixed with this commit.  After these changes, no null pointers were
> observed in the states array during testing.

LGTM, thanks.

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

> 
> ---
>  stdlib/tst-arc4random-thread.c      | 16 +++++++++++++++-
>  sysdeps/unix/sysv/linux/getrandom.c | 26 +++++++++++++++++++++++---
>  2 files changed, 38 insertions(+), 4 deletions(-)
> 
> diff --git a/stdlib/tst-arc4random-thread.c b/stdlib/tst-arc4random-thread.c
> index b7889b653f..cb7c3514af 100644
> --- a/stdlib/tst-arc4random-thread.c
> +++ b/stdlib/tst-arc4random-thread.c
> @@ -49,7 +49,7 @@ static const int sizes[] = { 12, 15, 16, 17, 24, 31, max_size };
>  struct blob
>  {
>    unsigned int size;
> -  int thread_id;
> +  int thread_id;		/* -1 means after fork.  */
>    unsigned int index;
>    unsigned char bytes[max_size];
>  };
> @@ -323,6 +323,20 @@ do_test_func (const char *fname, void (*func)(unsigned char *, size_t))
>          }
>      }
>  
> +  for (struct blob *p = dynarray_blob_begin (&global_result);
> +       p < end; ++p)
> +    {
> +      unsigned int sum = 0;
> +      for (unsigned int i = 0; i < p->size; ++i)
> +	sum += p->bytes[i];
> +      if (sum == 0)
> +	{
> +          support_record_failure ();
> +	  printf ("error: all-zero result of length %u on thread %d\n",
> +		  p->size, p->thread_id);
> +	}
> +    }
> +
>    dynarray_blob_free (&global_result);
>  
>    return 0;

Ok.

> diff --git a/sysdeps/unix/sysv/linux/getrandom.c b/sysdeps/unix/sysv/linux/getrandom.c
> index eab2b6806f..6b7be5ee97 100644
> --- a/sysdeps/unix/sysv/linux/getrandom.c
> +++ b/sysdeps/unix/sysv/linux/getrandom.c
> @@ -168,6 +168,11 @@ vgetrandom_get_state (void)
>    if (grnd_alloc.len > 0 || vgetrandom_get_state_alloc ())
>      state = grnd_alloc.states[--grnd_alloc.len];
>  
> +  /* Barrier needed by fork: The state must be gone from the array
> +     through len update before it becomes visible in the TCB.  (There
> +     is also a release barrier implied by the unlock, but issue a
> +     stronger barrier to help fork.)  */
> +  atomic_thread_fence_seq_cst ();
>    __libc_lock_unlock (grnd_alloc.lock);
>    internal_signal_restore_set (&set);
>  
> @@ -278,7 +283,10 @@ void
>  __getrandom_reset_state (struct pthread *curp)
>  {
>  #ifdef HAVE_GETRANDOM_VSYSCALL
> -  if (grnd_alloc.states == NULL || curp->getrandom_buf == NULL)
> +  /* The pointer can be reserved if the fork happened during a
> +     getrandom call.  */
> +  void *buf = release_ptr (curp->getrandom_buf);
> +  if (grnd_alloc.states == NULL || buf == NULL)
>      return;

Hum I have not considered this case, but it makes sense.

>    assert (grnd_alloc.len < grnd_alloc.cap);
>    grnd_alloc.states[grnd_alloc.len++] = release_ptr (curp->getrandom_buf);
> @@ -294,11 +302,23 @@ void
>  __getrandom_vdso_release (struct pthread *curp)
>  {
>  #ifdef HAVE_GETRANDOM_VSYSCALL
> -  if (curp->getrandom_buf == NULL)
> +  /* The pointer can be reserved if the thread was canceled in a
> +     signal handler.  */
> +  void *buf = release_ptr (curp->getrandom_buf);
> +  if (buf == NULL)
>      return;

Another catch, thanks.

>  
>    __libc_lock_lock (grnd_alloc.lock);
> -  grnd_alloc.states[grnd_alloc.len++] = curp->getrandom_buf;
> +
> +  size_t len = grnd_alloc.len;
> +  grnd_alloc.states[len] = curp->getrandom_buf;
> +  curp->getrandom_buf = NULL;
> +  /* Barrier needed by fork: The state must vanish from the TCB before
> +     it becomes visible in the states array.  Also avoid exposing the
> +     previous entry value at the same index in the states array (which
> +     may be in use by another thread).  */
> +  atomic_thread_fence_seq_cst ();
> +  grnd_alloc.len = len + 1;
>    __libc_lock_unlock (grnd_alloc.lock);
>  #endif
>  }
> 
> base-commit: 252fc3628bc2dd66b38dff7b5c22432bb34a8829
>