lkml.org 
[lkml]   [2019]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ALSA: hda: check RIRB to avoid use NULL pointer


On 04/30/2019 03:31 PM, Takashi Iwai wrote:
> On Tue, 30 Apr 2019 08:10:53 +0200,
> Song liwei wrote:
>>
>> From: Liwei Song <liwei.song@windriver.com>
>>
>> Fix the following BUG:
>>
>> BUG: unable to handle kernel NULL pointer dereference at 000000000000000c
>> Workqueue: events azx_probe_work [snd_hda_intel]
>> RIP: 0010:snd_hdac_bus_update_rirb+0x80/0x160 [snd_hda_core]
>> Call Trace:
>> <IRQ>
>> azx_interrupt+0x78/0x140 [snd_hda_codec]
>> __handle_irq_event_percpu+0x49/0x300
>> handle_irq_event_percpu+0x23/0x60
>> handle_irq_event+0x3c/0x60
>> handle_edge_irq+0xdb/0x180
>> handle_irq+0x23/0x30
>> do_IRQ+0x6a/0x140
>> common_interrupt+0xf/0xf
>>
>> The Call Trace happened when run kdump on a NFS rootfs system.
>> Exist the following calling sequence when boot the second kernel:
>>
>> azx_first_init()
>> --> azx_acquire_irq()
>> <-- interrupt come in, azx_interrupt() was called
>> --> hda_intel_init_chip()
>> --> azx_init_chip()
>> --> snd_hdac_bus_init_chip()
>> --> snd_hdac_bus_init_cmd_io();
>> --> init rirb.buf and corb.buf
>>
>> Interrupt happened after azx_acquire_irq() while RIRB still didn't got
>> initialized, then NULL pointer will be used when process the interrupt.
>>
>> Check the value of RIRB to ensure it is not NULL, to aviod some special
>> case may hang the system.
>>
>> Fixes: 14752412721c ("ALSA: hda - Add the controller helper codes to hda-core module")
>> Signed-off-by: Liwei Song <liwei.song@windriver.com>
>
> Oh, that's indeed a race there.
>
> But I guess the check introduced by the patch is still error-prone.
> Basically the interrupt handling should be moved after the chip
> initialization. I suppose that your platform uses the shared
> interrupt, not the MSI?

This is the information from /proc/interrupt
134: 0 102 0 0 IR-PCI-MSI 514048-edge snd_hda_intel:card0


>
> In anyway, alternative (and likely more certain) fix would be to move
> the azx_acquir_irq() call like the patch below (note: totally
> untested). Could you check whether it works?

Yes, It works.

Considering a previous patch like the one you provide will import some issue,
so I choose check the invalid value to low the risk, but just as you mentioned,
It is not a good solution.

commit 542cedec53c9e8b73f3f05bf8468823598c50489
Author: Yu Zhao <yuzhao@google.com>
Date: Tue Sep 11 15:12:46 2018 -0600

Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"

This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.

The patch doesn't fix accessing memory with null pointer in
skl_interrupt().

There are two problems: 1) skl_init_chip() is called twice, before
and after dma buffer is allocate. The first call sets bus->chip_init
which prevents the second from initializing bus->corb.buf and
rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
There is a small window which skl_interrupt() can be called if irq
has been acquired. If so, it crashes when using null dma buffer
pointers.

Thanks,
Liwei.


>
>
> thanks,
>
> Takashi
>
>
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1788,9 +1788,6 @@ static int azx_first_init(struct azx *chip)
> chip->msi = 0;
> }
>
> - if (azx_acquire_irq(chip, 0) < 0)
> - return -EBUSY;
> -
> pci_set_master(pci);
> synchronize_irq(bus->irq);
>
> @@ -1904,6 +1901,9 @@ static int azx_first_init(struct azx *chip)
> return -ENODEV;
> }
>
> + if (azx_acquire_irq(chip, 0) < 0)
> + return -EBUSY;
> +
> strcpy(card->driver, "HDA-Intel");
> strlcpy(card->shortname, driver_short_names[chip->driver_type],
> sizeof(card->shortname));
>
>

\
 
 \ /
  Last update: 2019-04-30 10:37    [W:0.380 / U:0.356 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site