| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <58453D33.5000006@mentor.com>
Date: Mon, 5 Dec 2016 02:10:59 -0800
From: Jiada Wang <jiada_wang@...tor.com>
To: Takashi Iwai <tiwai@...e.de>
CC: <perex@...ex.cz>, <alsa-devel@...a-project.org>,
<apape@...adit-jv.com>, <Mark_Craske@...tor.com>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3 v3] ALSA: usb-audio: fix race in snd_usb_endpoint_stop
Hi, Takashi
On 11/30/2016 01:00 AM, Takashi Iwai wrote:
> On Wed, 30 Nov 2016 08:59:23 +0100,
> Jiada Wang wrote:
>> From: Mark Craske<Mark_Craske@...tor.com>
>>
>> Kernel crash seen once:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address 00000008
>> pgd = a1d7c000
>> [00000008] *pgd=31c93831, *pte=00000000, *ppte=00000000
>> Internal error: Oops: 17 [#1] PREEMPT SMP ARM
>> CPU: 0 PID: 250 Comm: dbus-daemon Not tainted 3.14.51-03479-gf50bdf4 #1
>> task: a3ae61c0 ti: a08c8000 task.ti: a08c8000
>> PC is at retire_capture_urb+0x10/0x1f4 [snd_usb_audio]
>> LR is at snd_complete_urb+0x140/0x1f0 [snd_usb_audio]
>> pc : [<7f0eb22c>] lr : [<7f0e57fc>] psr: 200e0193
>> sp : a08c9c98 ip : a08c9ce8 fp : a08c9ce4
>> r10: 0000000a r9 : 00000102 r8 : 94cb3000
>> r7 : 94cb3000 r6 : 94d0f000 r5 : 94d0e8e8 r4 : 94d0e000
>> r3 : 7f0eb21c r2 : 00000000 r1 : 94cb3000 r0 : 00000000
>> Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user
>> Control: 10c5387d Table: 31d7c04a DAC: 00000015
>> Process dbus-daemon (pid: 250, stack limit = 0xa08c8238)
>> Stack: (0xa08c9c98 to 0xa08ca000)
>> ...
>> Backtrace:
>> [<7f0eb21c>] (retire_capture_urb [snd_usb_audio]) from [<7f0e57fc>] (snd_complete_urb+0x140/0x1f0 [snd_usb_audio])
>> [<7f0e56bc>] (snd_complete_urb [snd_usb_audio]) from [<80371118>] (__usb_hcd_giveback_urb+0x78/0xf4)
>> [<803710a0>] (__usb_hcd_giveback_urb) from [<80371514>] (usb_giveback_urb_bh+0x8c/0xc0)
>> [<80371488>] (usb_giveback_urb_bh) from [<80028e3c>] (tasklet_hi_action+0xc4/0x148)
>> [<80028d78>] (tasklet_hi_action) from [<80028358>] (__do_softirq+0x190/0x380)
>> [<800281c8>] (__do_softirq) from [<80028858>] (irq_exit+0x8c/0xfc)
>> [<800287cc>] (irq_exit) from [<8000ea88>] (handle_IRQ+0x8c/0xc8)
>> [<8000e9fc>] (handle_IRQ) from [<800085e8>] (gic_handle_irq+0xbc/0xf8)
>> [<8000852c>] (gic_handle_irq) from [<80509044>] (__irq_svc+0x44/0x78)
>> [<80508820>] (_raw_spin_unlock_irq) from [<8004b880>] (finish_task_switch+0x5c/0x100)
>> [<8004b824>] (finish_task_switch) from [<805052f0>] (__schedule+0x48c/0x6d8)
>> [<80504e64>] (__schedule) from [<805055d4>] (schedule+0x98/0x9c)
>> [<8050553c>] (schedule) from [<800116c8>] (do_work_pending+0x30/0xd0)
>> [<80011698>] (do_work_pending) from [<8000e160>] (work_pending+0xc/0x20)
>> Code: e1a0c00d e92ddff0 e24cb004 e24dd024 (e5902008)
>> Kernel panic - not syncing: Fatal exception in interrupt
>>
>> There is a race between retire_capture_urb()& stop_endpoints() which is
>> setting ep->data_subs to NULL. The underlying cause is in
>> snd_usb_endpoint_stop(), which sets
>> ep->data_subs = NULL;
>> ep->sync_slave = NULL;
>> ep->retire_data_urb = NULL;
>> ep->prepare_data_urb = NULL;
>>
>> An improvement, suggested by Andreas Pape (ADIT) is to read parameters
>> into local variables. This should solve race between stop and retire
>> where it is legal to store the pointers to local variable as they are
>> not freed in stop path but just set to NULL.
> Well, it's tricky. A saner way would be to clear the stuff really
> after all users are gone.
>
> An untested patch is below. Let me know if it really works.
Thanks for your proposal, I am afraid, we only found the race issue once
during
our automation test, so I can't test for your patch,
but from what I can see, your patch makes sense to me.
The only difference when apply with your patch is, now in case
stop_endpoints() is called
from TRIGGER_STOP, these stuff won't get cleared, but I think this isn't
an issue, is it?
Thanks,
Jiada
>
>> However, additional races may still happen in close+hw_free against
>> retire, as there pointers may be freed in addition. For example, while
>> in retire_capture_urb() runtime->dma_area might be freed/nulled.
> This shouldn't be a problem, as stop_endpoints() waits until all
> active URBS get killed.
>
>
> thanks,
>
> Takashi
>
> ---
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index c470251cea4b..f3fce9abece9 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -384,6 +384,9 @@ static void snd_complete_urb(struct urb *urb)
> if (unlikely(atomic_read(&ep->chip->shutdown)))
> goto exit_clear;
>
> + if (unlikely(!test_bit(EP_FLAG_RUNNING,&ep->flags)))
> + goto exit_clear;
> +
> if (usb_pipeout(ep->pipe)) {
> retire_outbound_urb(ep, ctx);
> /* can be stopped during retire callback */
> @@ -534,6 +537,11 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep)
> alive, ep->ep_num);
> clear_bit(EP_FLAG_STOPPING,&ep->flags);
>
> + ep->data_subs = NULL;
> + ep->sync_slave = NULL;
> + ep->retire_data_urb = NULL;
> + ep->prepare_data_urb = NULL;
> +
> return 0;
> }
>
> @@ -1006,10 +1014,6 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep)
>
> if (--ep->use_count == 0) {
> deactivate_urbs(ep, false);
> - ep->data_subs = NULL;
> - ep->sync_slave = NULL;
> - ep->retire_data_urb = NULL;
> - ep->prepare_data_urb = NULL;
> set_bit(EP_FLAG_STOPPING,&ep->flags);
> }
> }
Powered by blists - more mailing lists