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_loopback_active_get.config > > 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@xxxxxxx> 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@xxxxxxxxx> Cc: <stable@xxxxxxxxxxxxxxx> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> --- 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; } -- 2.16.3 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
