Re: [PATCH] ALSA: seq: 2nd attempt at fixing race creating a q

[Takashi Iwai <tiwai@suse.de>]

On Fri, 11 Aug 2017 05:07:34 +0200,
Daniel Mentz wrote:
> 
> commit 4842e98f26dd80be3623c4714a244ba52ea096a8 ("ALSA: seq: Fix race at
> creating a queue") attempted to fix a race reported by syzkaller. That
> fix has been described as follows:
> 
> "
> When a sequencer queue is created in snd_seq_queue_alloc(),it adds the
> new queue element to the public list before referencing it.  Thus the
> queue might be deleted before the call of snd_seq_queue_use(), and it
> results in the use-after-free error, as spotted by syzkaller.
> 
> The fix is to reference the queue object at the right time.
> "
> 
> The last phrase in that commit message refers to calling queue_use(q, client,
> 1) which increments queue->clients, but that does not prevent the
> DELETE_QUEUE ioctl() and queue_delete() from kfree()ing the queue.
> Hence, the commit is not effective at preventing the race.

kfree() is performed only after snd_use_lock_sync(), thus by acquiring
snd_use_lock() it doesn't race.  If it were already deleted,
queue_use() returns NULL so it's also fine.

Or do you actually see any crash or the wild behavior?


thanks,

Takashi

> 
> This commit adds code to effectively prevent the removal of the queue by
> calling snd_use_lock_use().
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: <stable@vger.kernel.org>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> ---
>  sound/core/seq/seq_clientmgr.c | 13 ++++---------
>  sound/core/seq/seq_queue.c     | 14 +++++++++-----
>  sound/core/seq/seq_queue.h     |  2 +-
>  3 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c
> index 272c55fe17c8..ea2d0ae85bd3 100644
> --- a/sound/core/seq/seq_clientmgr.c
> +++ b/sound/core/seq/seq_clientmgr.c
> @@ -1502,16 +1502,11 @@ static int snd_seq_ioctl_unsubscribe_port(struct snd_seq_client *client,
>  static int snd_seq_ioctl_create_queue(struct snd_seq_client *client, void *arg)
>  {
>  	struct snd_seq_queue_info *info = arg;
> -	int result;
>  	struct snd_seq_queue *q;
>  
> -	result = snd_seq_queue_alloc(client->number, info->locked, info->flags);
> -	if (result < 0)
> -		return result;
> -
> -	q = queueptr(result);
> -	if (q == NULL)
> -		return -EINVAL;
> +	q = snd_seq_queue_alloc(client->number, info->locked, info->flags);
> +	if (IS_ERR(q))
> +		return PTR_ERR(q);
>  
>  	info->queue = q->queue;
>  	info->locked = q->locked;
> @@ -1521,7 +1516,7 @@ static int snd_seq_ioctl_create_queue(struct snd_seq_client *client, void *arg)
>  	if (!info->name[0])
>  		snprintf(info->name, sizeof(info->name), "Queue-%d", q->queue);
>  	strlcpy(q->name, info->name, sizeof(q->name));
> -	queuefree(q);
> +	snd_use_lock_free(&q->use_lock);
>  
>  	return 0;
>  }
> diff --git a/sound/core/seq/seq_queue.c b/sound/core/seq/seq_queue.c
> index 450c5187eecb..79e0c5604ef8 100644
> --- a/sound/core/seq/seq_queue.c
> +++ b/sound/core/seq/seq_queue.c
> @@ -184,22 +184,26 @@ void __exit snd_seq_queues_delete(void)
>  static void queue_use(struct snd_seq_queue *queue, int client, int use);
>  
>  /* allocate a new queue -
> - * return queue index value or negative value for error
> + * return pointer to new queue or ERR_PTR(-errno) for error
> + * The new queue's use_lock is set to 1. It is the caller's responsibility to
> + * call snd_use_lock_free(&q->use_lock).
>   */
> -int snd_seq_queue_alloc(int client, int locked, unsigned int info_flags)
> +struct snd_seq_queue *snd_seq_queue_alloc(int client, int locked, unsigned int info_flags)
>  {
>  	struct snd_seq_queue *q;
>  
>  	q = queue_new(client, locked);
>  	if (q == NULL)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  	q->info_flags = info_flags;
>  	queue_use(q, client, 1);
> +	snd_use_lock_use(&q->use_lock);
>  	if (queue_list_add(q) < 0) {
> +		snd_use_lock_free(&q->use_lock);
>  		queue_delete(q);
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  	}
> -	return q->queue;
> +	return q;
>  }
>  
>  /* delete a queue - queue must be owned by the client */
> diff --git a/sound/core/seq/seq_queue.h b/sound/core/seq/seq_queue.h
> index 30c8111477f6..719093489a2c 100644
> --- a/sound/core/seq/seq_queue.h
> +++ b/sound/core/seq/seq_queue.h
> @@ -71,7 +71,7 @@ void snd_seq_queues_delete(void);
>  
>  
>  /* create new queue (constructor) */
> -int snd_seq_queue_alloc(int client, int locked, unsigned int flags);
> +struct snd_seq_queue *snd_seq_queue_alloc(int client, int locked, unsigned int flags);
>  
>  /* delete queue (destructor) */
>  int snd_seq_queue_delete(int client, int queueid);
> -- 
> 2.14.0.434.g98096fd7a8-goog
>