From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7C656C433ED for ; Fri, 7 May 2021 09:24:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3F7726144F for ; Fri, 7 May 2021 09:24:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235912AbhEGJZH (ORCPT ); Fri, 7 May 2021 05:25:07 -0400 Received: from mx2.suse.de ([195.135.220.15]:56006 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235202AbhEGJZF (ORCPT ); Fri, 7 May 2021 05:25:05 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 1A6F8AC3E; Fri, 7 May 2021 09:24:05 +0000 (UTC) Date: Fri, 07 May 2021 11:24:04 +0200 Message-ID: From: Takashi Iwai To: =?UTF-8?B?0J/QvtC00LPQvtGA0L3Ri9C5INCQ0LvQtdC60YHQtdC5INCe0LvQtdCz0L4=?= =?UTF-8?B?0LLQuNGH?= Cc: Jaroslav Kysela , Takashi Iwai , Leon Romanovsky , Pierre-Louis Bossart , Alexey Khoroshilov , 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: References: 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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