On Mon, 30 Apr 2018 09:53:14 +0200, DaeRyong Jeong wrote:
We report the crash: KASAN: use-after-free in loopback_active_get
This crash has been found in v4.17-rc1 using RaceFuzzer (a modified version of Syzkaller), which we describe more at the end of this report. Our analysis shows that the race occurs when invoking two syscalls concurrently, ioctl$SNDRV_CTL_IOCTL_ELEM_READ and syz_open_dev$audion.
kernel config: https://kiwi.cs.purdue.ed/static/race-fuzzer/KASAN_use-after-free_in_loopbac...
Analysis:
When there is a race between sound/drivers/aloop.c:895 (loopback_active_get) and sound/drivers/aloop.c:678 (free_cable), the retrieved cable pointer in loopback_active_get() may point to the freed memory region. When loopback_active_get() dereferences this pointer, use-after-free occurs.
Possible CPU execution:
CPU0 CPU1 loopback_active_get() free_cable()
struct loopback_cable *cable = ... loopback->cables[substream->number][dev] = NULL; kfree(cable); cable->running <-- Use-after-free
Call Sequence: CPU0 loopback_active_get snd_ctl_elem_read snd_ctl_elem_read_user snd_ctl_ioctl
CPU1 free_cable loopback_close snd_pcm_release_substream snd_pcm_release_substream snd_pcm_oss_release_file snd_pcm_oss_release_file snd_pcm_oss_release
We observed that snd_pcm_oss_release() is called during the open("/dev/audio1") syscall. In our configuration, the function do_dentry_open() returns -EINVAL since below if statement is evaluated as true. if ((f->f_flags & O_DIRECT) (!f->f_mapping->a_ops || !f->f_mapping->a_ops->direct_IO)) Therefore, fput() is called and snd_pcm_oss_release() is called as a pending work before returning to the user space. But we suspect that the insufficient locking between snd_ctl_ioctl() and snd_pcm_oss_release(), not the vfs_layer, is the cause of the crash.
Right, it must be a race between ALSA ctl API ioctl access and the PCM release. It's irrelevant with whether it's a delayed release or not, but just a simple fact that it misses the loopback->cable_lock at accessing.
The patch below should fix the issue. Could you check it?
thanks,
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: aloop: Add missing cable lock to ctl API callbacks
Some control API callbacks in aloop driver are too lazy to take the loopback->cable_lock and it results in possible races of cable access while it's being freed. It eventually lead to a UAF, as reported by fuzzer recently.
This patch covers such control API callbacks and add the proper mutex locks.
Reported-by: DaeRyong Jeong threeearcat@gmail.com Cc: stable@vger.kernel.org Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/drivers/aloop.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 58e349fc893f..eab7f594ebe7 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -831,9 +831,11 @@ static int loopback_rate_shift_get(struct snd_kcontrol *kcontrol, { struct loopback *loopback = snd_kcontrol_chip(kcontrol); + mutex_lock(&loopback->cable_lock); ucontrol->value.integer.value[0] = loopback->setup[kcontrol->id.subdevice] [kcontrol->id.device].rate_shift; + mutex_unlock(&loopback->cable_lock); return 0; }
@@ -865,9 +867,11 @@ static int loopback_notify_get(struct snd_kcontrol *kcontrol, { struct loopback *loopback = snd_kcontrol_chip(kcontrol); + mutex_lock(&loopback->cable_lock); ucontrol->value.integer.value[0] = loopback->setup[kcontrol->id.subdevice] [kcontrol->id.device].notify; + mutex_unlock(&loopback->cable_lock); return 0; }
@@ -879,12 +883,14 @@ static int loopback_notify_put(struct snd_kcontrol *kcontrol, int change = 0;
val = ucontrol->value.integer.value[0] ? 1 : 0; + mutex_lock(&loopback->cable_lock); if (val != loopback->setup[kcontrol->id.subdevice] [kcontrol->id.device].notify) { loopback->setup[kcontrol->id.subdevice] [kcontrol->id.device].notify = val; change = 1; } + mutex_unlock(&loopback->cable_lock); return change; }
@@ -892,15 +898,18 @@ static int loopback_active_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct loopback *loopback = snd_kcontrol_chip(kcontrol); - struct loopback_cable *cable = loopback->cables - [kcontrol->id.subdevice][kcontrol->id.device ^ 1]; + struct loopback_cable *cable; + unsigned int val = 0;
+ mutex_lock(&loopback->cable_lock); + cable = loopback->cables[kcontrol->id.subdevice][kcontrol->id.device ^ 1]; if (cable != NULL) { unsigned int running = cable->running ^ cable->pause;
val = (running & (1 << SNDRV_PCM_STREAM_PLAYBACK)) ? 1 : 0; } + mutex_unlock(&loopback->cable_lock); ucontrol->value.integer.value[0] = val; return 0; } @@ -943,9 +952,11 @@ static int loopback_rate_get(struct snd_kcontrol *kcontrol, { struct loopback *loopback = snd_kcontrol_chip(kcontrol); + mutex_lock(&loopback->cable_lock); ucontrol->value.integer.value[0] = loopback->setup[kcontrol->id.subdevice] [kcontrol->id.device].rate; + mutex_unlock(&loopback->cable_lock); return 0; }
@@ -965,9 +976,11 @@ static int loopback_channels_get(struct snd_kcontrol *kcontrol, { struct loopback *loopback = snd_kcontrol_chip(kcontrol); + mutex_lock(&loopback->cable_lock); ucontrol->value.integer.value[0] = loopback->setup[kcontrol->id.subdevice] [kcontrol->id.device].channels; + mutex_unlock(&loopback->cable_lock); return 0; }