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 <dvyu...@google.com>
> Cc: <sta...@vger.kernel.org>
> Cc: Takashi Iwai <ti...@suse.de>
> Signed-off-by: Daniel Mentz <danielme...@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
>
