From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936440AbdEXHXE (ORCPT ); Wed, 24 May 2017 03:23:04 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:35816 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933181AbdEXHXC (ORCPT ); Wed, 24 May 2017 03:23:02 -0400 Date: Wed, 24 May 2017 08:22:58 +0100 From: Lee Jones To: "Gustavo A. R. Silva" Cc: patches@opensource.wolfsonmicro.com, linux-kernel@vger.kernel.org Subject: Re: [mfd] question about potential null pointer dereference Message-ID: <20170524072258.7qnnnrdodjwc55lk@dell> References: <20170523163341.Horde.9ys90CmTnWtlq_NxAO5TqfN@gator4166.hostgator.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170523163341.Horde.9ys90CmTnWtlq_NxAO5TqfN@gator4166.hostgator.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 23 May 2017, Gustavo A. R. Silva wrote: > > Hello everybody, > > While looking into Coverity ID 1408830 I ran into the following piece of > code at drivers/mfd/wm831x-spi.c:26 > > 26static int wm831x_spi_probe(struct spi_device *spi) > 27{ > 28 struct wm831x_pdata *pdata = dev_get_platdata(&spi->dev); > 29 const struct spi_device_id *id = spi_get_device_id(spi); > 30 const struct of_device_id *of_id; > 31 struct wm831x *wm831x; > 32 enum wm831x_parent type; > 33 int ret; > 34 > 35 if (spi->dev.of_node) { > 36 of_id = of_match_device(wm831x_of_match, &spi->dev); > 37 type = (enum wm831x_parent)of_id->data; > 38 } else { > 39 type = (enum wm831x_parent)id->driver_data; > 40 } > 41 > 42 wm831x = devm_kzalloc(&spi->dev, sizeof(struct wm831x), > GFP_KERNEL); > 43 if (wm831x == NULL) > 44 return -ENOMEM; > 45 > 46 spi->mode = SPI_MODE_0; > 47 > 48 spi_set_drvdata(spi, wm831x); > 49 wm831x->dev = &spi->dev; > 50 wm831x->type = type; > 51 > 52 wm831x->regmap = devm_regmap_init_spi(spi, &wm831x_regmap_config); > 53 if (IS_ERR(wm831x->regmap)) { > 54 ret = PTR_ERR(wm831x->regmap); > 55 dev_err(wm831x->dev, "Failed to allocate register map: > %d\n", > 56 ret); > 57 return ret; > 58 } > 59 > 60 if (pdata) > 61 memcpy(&wm831x->pdata, pdata, sizeof(*pdata)); > 62 > 63 return wm831x_device_init(wm831x, spi->irq); > 64} > > The issue here is that there is a potential NULL pointer dereference at line > 37, in case function of_match_device() returns NULL. > > Maybe a patch like the following could be applied in order to avoid any > chance of a NULL pointer dereference: I do not believe it's possible for of_match_device() to return NULL in this case. However, if you wanted to submit a patch checking for it, it would not be rejected. > index c332e28..7b227c9 100644 > --- a/drivers/mfd/wm831x-spi.c > +++ b/drivers/mfd/wm831x-spi.c > @@ -34,6 +34,10 @@ static int wm831x_spi_probe(struct spi_device *spi) > > if (spi->dev.of_node) { > of_id = of_match_device(wm831x_of_match, &spi->dev); > + if (!of_id) { > + dev_err(&spi->dev, "Failed to find matching id\n"); "Failed to match device" > + return -EINVAL; -ENODEV > type = (enum wm831x_parent)of_id->data; > } else { > type = (enum wm831x_parent)id->driver_data; > > What do you think? > > I'd really appreciate any comment on this. > > Thank you! -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog