Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 07 May 2021 11:24:04 +0200
Message-ID: <s5hfsyyj3rf.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     =?UTF-8?B?0J/QvtC00LPQvtGA0L3Ri9C5INCQ0LvQtdC60YHQtdC5INCe0LvQtdCz0L4=?=
         =?UTF-8?B?0LLQuNGH?= <s02190176@gse.cs.msu.ru>
Cc:     Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        Leon Romanovsky <leon@kernel.org>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Alexey Khoroshilov <khoroshilov@ispras.ru>,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        ldv-project@linuxtesting.org
Subject: Re: [BUG] ALSA: korg1212: Potential NULL pointer dereference in snd_korg1212_interrupt()
In-Reply-To: <CAA5EkfcVXC4MwMj=2GsfyLDjPsnT1nxgNk5XhdE346MuLFwMVQ@mail.gmail.com>
References: <CAA5EkfcVXC4MwMj=2GsfyLDjPsnT1nxgNk5XhdE346MuLFwMVQ@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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