Segfault in smbd: messaging_dgm_fde_active

[Jeremy Allison]

On Mon, Jul 09, 2018 at 02:06:05PM +0200, Stefan Metzmacher via samba-technical wrote:
> Hi,
> 
> > Ping!
> > 
> > I know it is the crunch before the release but it would be good not to
> > forget this.
> 
> I looked at it and found the problem.
> 
> With auth_winbind as AD DC, smbd creates two temporary
> imessaging_context structures, both using messaging_dgm_ref()
> internally.
> 
> Then we free both of them when the authentication is done.
> So 'next' becomes a stale pointer in msg_dgm_ref_recv().
> 
> The attached patch fixes this. I'm currently running
> a private autobuild with it.
> 
> Please review and push:-)

OK, I'm reviewing this now, and I think I understand
it, but once I'm done I might ask you to add some
clarification to the commit message (or not, depending
on how easy this is to understand :-).

Cheers,

	Jeremy.

> From b4a5833fbd8a804822f2934f397d818302870a51 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Mon, 9 Jul 2018 12:33:34 +0200
> Subject: [PATCH] s3:messages: make the loop in msg_dgm_ref_recv() more robust
>  against stale pointers
> 
> The interaction between msg_dgm_ref_recv() and msg_dgm_ref_destructor()
> doesn't allow two references from messaging_dgm_ref() to be free'd
> during the loop in msg_dgm_ref_recv().
> 
> In addition to the global 'refs' list, we also need to
> have a global 'next_ref' pointer, which can be cleared in
> msg_dgm_ref_destructor().
> 
> As AD DC we hit this when using irpc in auth_winbind,
> which uses imessaging_client_init().
> In addition to the main messaging_dgm_ref() in smbd,
> source3/auth/auth_samba4.c: prepare_gensec() and
> make_auth4_context_s4() also generate a temporary
> imessaging_context for auth_context->msg_ctx from within
> auth_generic_prepare().
> 
> Bug: https://bugzilla.samba.org/show_bug.cgi?id=13514
> 
> Signed-off-by: Stefan Metzmacher <metze at samba.org>
> ---
>  source3/lib/messages_dgm_ref.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/source3/lib/messages_dgm_ref.c b/source3/lib/messages_dgm_ref.c
> index 39d22700740c..4cf80a789987 100644
> --- a/source3/lib/messages_dgm_ref.c
> +++ b/source3/lib/messages_dgm_ref.c
> @@ -35,6 +35,7 @@ struct msg_dgm_ref {
>  
>  static pid_t dgm_pid = 0;
>  static struct msg_dgm_ref *refs = NULL;
> +static struct msg_dgm_ref *next_ref = NULL;
>  
>  static int msg_dgm_ref_destructor(struct msg_dgm_ref *r);
>  static void msg_dgm_ref_recv(struct tevent_context *ev,
> @@ -121,16 +122,16 @@ static void msg_dgm_ref_recv(struct tevent_context *ev,
>  			     const uint8_t *msg, size_t msg_len,
>  			     int *fds, size_t num_fds, void *private_data)
>  {
> -	struct msg_dgm_ref *r, *next;
> +	struct msg_dgm_ref *r;
>  
>  	/*
>  	 * We have to broadcast incoming messages to all refs. The first ref
>  	 * that grabs the fd's will get them.
>  	 */
> -	for (r = refs; r != NULL; r = next) {
> +	for (r = refs; r != NULL; r = next_ref) {
>  		bool active;
>  
> -		next = r->next;
> +		next_ref = r->next;
>  
>  		active = messaging_dgm_fde_active(r->fde);
>  		if (!active) {
> @@ -152,6 +153,10 @@ static int msg_dgm_ref_destructor(struct msg_dgm_ref *r)
>  	}
>  	DLIST_REMOVE(refs, r);
>  
> +	if (r == next_ref) {
> +		next_ref = NULL;
> +	}
> +
>  	TALLOC_FREE(r->fde);
>  
>  	DBG_DEBUG("refs=%p\n", refs);
> -- 
> 2.17.1
>