linux.git/sound/core/timer.c, branch v4.5

ALSA: timer: Fix race at concurrent reads

2016-02-09T11:23:42+00:00

snd_timer_user_read() has a potential race among parallel reads, as
qhead and qused are updated outside the critical section due to
copy_to_user() calls.  Move them into the critical section, and also
sanitize the relevant code a bit.

Cc: 
Signed-off-by: Takashi Iwai

ALSA: timer: Fix race between stop and interrupt

2016-02-09T11:02:32+00:00

A slave timer element also unlinks at snd_timer_stop() but it takes
only slave_active_lock.  When a slave is assigned to a master,
however, this may become a race against the master's interrupt
handling, eventually resulting in a list corruption.  The actual bug
could be seen with a syzkaller fuzzer test case in BugLink below.

As a fix, we need to take timeri->timer->lock when timer isn't NULL,
i.e. assigned to a master, while the assignment to a master itself is
protected by slave_active_lock.

BugLink: http://lkml.kernel.org/r/CACT4Y+Y_Bm+7epAb=8Wi=AaWd+DYS7qawX52qxdCfOfY49vozQ@mail.gmail.com
Cc: 
Signed-off-by: Takashi Iwai

ALSA: timer: Fix wrong instance passed to slave callbacks

2016-02-08T16:40:08+00:00

In snd_timer_notify1(), the wrong timer instance was passed for slave
ccallback function.  This leads to the access to the wrong data when
an incompatible master is handled (e.g. the master is the sequencer
timer and the slave is a user timer), as spotted by syzkaller fuzzer.

This patch fixes that wrong assignment.

BugLink: http://lkml.kernel.org/r/CACT4Y+Y_Bm+7epAb=8Wi=AaWd+DYS7qawX52qxdCfOfY49vozQ@mail.gmail.com
Reported-by: Dmitry Vyukov 
Cc: 
Signed-off-by: Takashi Iwai

ALSA: timer: Fix leftover link at closing

2016-02-04T16:19:09+00:00

In ALSA timer core, the active timer instance is managed in
active_list linked list.  Each element is added / removed dynamically
at timer start, stop and in timer interrupt.  The problem is that
snd_timer_interrupt() has a thinko and leaves the element in
active_list when it's the last opened element.  This eventually leads
to list corruption or use-after-free error.

This hasn't been revealed because we used to delete the list forcibly
in snd_timer_stop() in the past.  However, the recent fix avoids the
double-stop behavior (in commit [f784beb75ce8: ALSA: timer: Fix link
corruption due to double start or stop]), and this leak hits reality.

This patch fixes the link management in snd_timer_interrupt().  Now it
simply unlinks no matter which stream is.

BugLink: http://lkml.kernel.org/r/CACT4Y+Yy2aukHP-EDp8-ziNqNNmb-NTf=jDWXMP7jB8HDa2vng@mail.gmail.com
Reported-by: Dmitry Vyukov 
Cc: 
Signed-off-by: Takashi Iwai

ALSA: timer: Sync timer deletion at closing the system timer

2016-02-02T23:15:42+00:00

ALSA timer core framework has no sync point at stopping because it's
called inside the spinlock.  Thus we need a sync point at close for
avoiding the stray timer task.  This is simply done by implementing
the close callback just calling del_timer_sync().  (It's harmless to
call it unconditionally, as the core timer itself cares of the already
deleted timer instance.)

Signed-off-by: Takashi Iwai

ALSA: timer: Fix link corruption due to double start or stop

2016-02-01T11:23:29+00:00

Although ALSA timer code got hardening for races, it still causes
use-after-free error.  This is however rather a corrupted linked list,
not actually the concurrent accesses.  Namely, when timer start is
triggered twice, list_add_tail() is called twice, too.  This ends
up with the link corruption and triggers KASAN error.

The simplest fix would be replacing list_add_tail() with
list_move_tail(), but fundamentally it's the problem that we don't
check the double start/stop correctly.  So, the right fix here is to
add the proper checks to snd_timer_start() and snd_timer_stop() (and
their variants).

BugLink: http://lkml.kernel.org/r/CACT4Y+ZyPRoMQjmawbvmCEDrkBD2BQuH7R09=eOkf5ESK8kJAw@mail.gmail.com
Reported-by: Dmitry Vyukov 
Cc: 
Signed-off-by: Takashi Iwai

ALSA: timer: Introduce disconnect op to snd_timer_instance

2016-01-21T16:51:42+00:00

Instead of the previous ugly hack, introduce a new op, disconnect, to
snd_timer_instance object for handling the wake up of pending tasks
more cleanly.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109431
Signed-off-by: Takashi Iwai

ALSA: timer: Handle disconnection more safely

2016-01-21T16:41:50+00:00

Currently ALSA timer device doesn't take the disconnection into
account very well; it merely unlinks the timer device at disconnection
callback but does nothing else.  Because of this, when an application
accessing the timer device is disconnected, it may release the
resource before actually closed.  In most cases, it results in a
warning message indicating a leftover timer instance like:
   ALSA: timer xxxx is busy?
But basically this is an open race.

This patch tries to address it.  The strategy is like other ALSA
devices: namely,
- Manage card's refcount at each open/close
- Wake up the pending tasks at disconnection
- Check the shutdown flag appropriately at each possible call

Note that this patch has one ugly hack to handle the wakeup of pending
tasks.  It'd be cleaner to introduce a new disconnect op to
snd_timer_instance ops.  But since it would lead to internal ABI
breakage and it eventually increase my own work when backporting to
stable kernels, I took a different path to implement locally in
timer.c.  A cleanup patch will follow at next for 4.5 kernel.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109431
Cc:  # v3.15+
Signed-off-by: Takashi Iwai

ALSA: timer: Code cleanup

2016-01-15T14:50:34+00:00

This is a minor code cleanup without any functional changes:
- Kill keep_flag argument from _snd_timer_stop(), as all callers pass
  only it false.
- Remove redundant NULL check in _snd_timer_stop().

Signed-off-by: Takashi Iwai

ALSA: timer: Harden slave timer list handling

2016-01-15T14:50:34+00:00

A slave timer instance might be still accessible in a racy way while
operating the master instance as it lacks of locking.  Since the
master operation is mostly protected with timer->lock, we should cope
with it while changing the slave instance, too.  Also, some linked
lists (active_list and ack_list) of slave instances aren't unlinked
immediately at stopping or closing, and this may lead to unexpected
accesses.

This patch tries to address these issues.  It adds spin lock of
timer->lock (either from master or slave, which is equivalent) in a
few places.  For avoiding a deadlock, we ensure that the global
slave_active_lock is always locked at first before each timer lock.

Also, ack and active_list of slave instances are properly unlinked at
snd_timer_stop() and snd_timer_close().

Last but not least, remove the superfluous call of _snd_timer_stop()
at removing slave links.  This is a noop, and calling it may confuse
readers wrt locking.  Further cleanup will follow in a later patch.

Actually we've got reports of use-after-free by syzkaller fuzzer, and
this hopefully fixes these issues.

Reported-by: Dmitry Vyukov 
Cc: 
Signed-off-by: Takashi Iwai