On Fri, 07 May 2021 11:18:56 +0200,
Подгорный Алексей Олегович wrote:
>
> snd_korg1212_create() makes the following steps during initialization
> of the card:
> 1) registers an interrupt handler (lines 2230-2232)
> 2) allocates and initializes korg1212->sharedBufferPtr (lines 2280-2287)
> 3) reboots the card via snd_korg1212_Send1212Command() (line 2358)
>
> 2145 static int snd_korg1212_create(struct snd_card *card, struct
> pci_dev *pci, struct snd_korg1212 **rchip)
> 2147 {
> ...
> 2230 err = request_irq(pci->irq, snd_korg1212_interrupt,
> 2231 IRQF_SHARED,
> 2232 KBUILD_MODNAME, korg1212);
> ...
> 2280 if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, &pci->dev,
> 2281 sizeof(struct KorgSharedBuffer), &korg1212->dma_shared) < 0){
>
> 2282 snd_printk(KERN_ERR "korg1212: can not
> allocate shared buffer memory (%zdbytes)\n",
> sizeof(struct KorgSharedBuffer));
>
> 2283 snd_korg1212_free(korg1212);
> 2284 return -ENOMEM;
> 2285 }
> 2286 korg1212->sharedBufferPtr =
> (struct KorgSharedBuffer*)korg1212->dma_shared.area;
> 2287 korg1212->sharedBufferPhy = korg1212->dma_shared.addr;
> ...
> 2358 rc = snd_korg1212_Send1212Command(korg1212,
> K1212_DB_RebootCard, 0, 0, 0, 0);
> ...
> 2412 }
>
> But if interrupt happens when snd_korg1212_create() is still within
> lines 2233-2286,
> snd_korg1212_interrupt() may dereference korg1212->sharedBufferPtr before
> it was initialized without any checks (line 1149):
>
> 1098 static irqreturn_t snd_korg1212_interrupt(int irq, void *dev_id)
> 1099 {
> ...
> 1116 switch (doorbellValue) {
> ...
> 1145 case K1212_DB_CARDSTOPPED:
> 1146 K1212_DEBUG_PRINTK_VERBOSE("K1212_DEBUG: IRQ
> CSTP count - %ld, %x, [%s].\n",
> 1147 korg1212->irqcount, doorbellValue,
> 1148 stateName[korg1212->cardState]);
> 1149 korg1212->sharedBufferPtr->cardCommand = 0;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 1150 break;
> ...
> 1185 }
>
> Should we be sure that such interrupt cannot happen or
> should we move the registration of the interrupt handler after
> korg1212->sharedBufferPtr is initialized?
Yes, in general the IRQ handler should be registered at the end.
Could you submit a fix patch?
thanks,
Takashi
