* Re: FW: iio: adc: ad7298: rework external ref setup & remove platform data
[not found] ` <DM6PR03MB4411E6C39D991B5C5AAC1F86F9F80@DM6PR03MB4411.namprd03.prod.outlook.com>
@ 2020-11-27 9:24 ` Alexandru Ardelean
2020-11-27 12:15 ` Jonathan Cameron
0 siblings, 1 reply; 2+ messages in thread
From: Alexandru Ardelean @ 2020-11-27 9:24 UTC (permalink / raw)
To: Alexandru Ardelean, Jonathan Cameron, colin.king, linux-iio
On Fri, Nov 27, 2020 at 8:24 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Colin Ian King <colin.king@canonical.com>
> > Sent: Wednesday, November 25, 2020 4:37 PM
> > To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; Jonathan
> > Cameron <Jonathan.Cameron@huawei.com>
> > Subject: re: iio: adc: ad7298: rework external ref setup & remove platform data
> >
> > [External]
> >
> > Hi there,
> >
> > Static analysis with Coverity on Linux-next today found a potential null pointer
> > dereference. I believe it was triggered by the following commit:
> >
> > commit 28963f2f6b46d75bda8fed15bd5ce9923427a40d
> > Author: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > Date: Thu Oct 1 17:10:48 2020 +0300
> >
> > iio: adc: ad7298: rework external ref setup & remove platform data
> >
> > ..however the issue may have existed before now and this change triggered the
> > bug being found.
> >
> > The analysis is as follows:
> >
> >
> > 282 static int ad7298_probe(struct spi_device *spi)
> > 283 {
> > 284 struct ad7298_state *st;
> > 285 struct iio_dev *indio_dev;
> > 286 int ret;
> > 287
> > 288 indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> >
> > 1. Condition indio_dev == NULL, taking false branch.
> >
> > 289 if (indio_dev == NULL)
> > 290 return -ENOMEM;
> > 291
> > 292 st = iio_priv(indio_dev);
> > 293
> > 294 st->reg = devm_regulator_get_optional(&spi->dev, "vref");
> >
> > 2. Condition !IS_ERR(st->reg), taking false branch.
> >
> > 295 if (!IS_ERR(st->reg)) {
> > 296 st->ext_ref = AD7298_EXTREF;
> > 297 } else {
> > 298 ret = PTR_ERR(st->reg);
> >
> > 3. Condition ret != -19, taking false branch.
> >
> > 299 if (ret != -ENODEV)
> > 300 return ret;
> > 301
> > 302 st->reg = NULL;
> > 303 }
> > 304
> >
> > 4. Condition st->reg, taking false branch.
> > 5. var_compare_op: Comparing st->reg to null implies that st->reg might be
> > null.
> >
> > 305 if (st->reg) {
> > 306 ret = regulator_enable(st->reg);
> > 307 if (ret)
> > 308 return ret;
> > 309 }
> > 310
> > 311 spi_set_drvdata(spi, indio_dev);
> > 312
> > 313 st->spi = spi;
> > 314
> > 315 indio_dev->name = spi_get_device_id(spi)->name;
> > 316 indio_dev->modes = INDIO_DIRECT_MODE;
> > 317 indio_dev->channels = ad7298_channels;
> > 318 indio_dev->num_channels = ARRAY_SIZE(ad7298_channels);
> > 319 indio_dev->info = &ad7298_info;
> > 320
> > 321 /* Setup default message */
> > 322
> > 323 st->scan_single_xfer[0].tx_buf = &st->tx_buf[0];
> > 324 st->scan_single_xfer[0].len = 2;
> > 325 st->scan_single_xfer[0].cs_change = 1;
> > 326 st->scan_single_xfer[1].tx_buf = &st->tx_buf[1];
> > 327 st->scan_single_xfer[1].len = 2;
> > 328 st->scan_single_xfer[1].cs_change = 1;
> > 329 st->scan_single_xfer[2].rx_buf = &st->rx_buf[0];
> > 330 st->scan_single_xfer[2].len = 2;
> > 331
> > 332 spi_message_init(&st->scan_single_msg);
> > 333 spi_message_add_tail(&st->scan_single_xfer[0],
> > &st->scan_single_msg);
> > 334 spi_message_add_tail(&st->scan_single_xfer[1],
> > &st->scan_single_msg);
> > 335 spi_message_add_tail(&st->scan_single_xfer[2],
> > &st->scan_single_msg);
> > 336
> > 337 ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > 338 &ad7298_trigger_handler, NULL);
> >
> > 6. Condition ret, taking true branch.
> >
> > 339 if (ret)
> >
> > 7. Jumping to label error_disable_reg.
> >
> > 340 goto error_disable_reg;
> > 341
> > 342 ret = iio_device_register(indio_dev);
> > 343 if (ret)
> > 344 goto error_cleanup_ring;
> > 345
> > 346 return 0;
> > 347
> > 348 error_cleanup_ring:
> > 349 iio_triggered_buffer_cleanup(indio_dev);
> > 350 error_disable_reg:
> >
> > 8. Condition st->ext_ref, taking true branch.
> >
> > 351 if (st->ext_ref)
> >
> > Dereference after null check (FORWARD_NULL)
> > 9. var_deref_model: Passing null pointer st->reg to regulator_disable, which
> > dereferences it.
> >
> > 352 regulator_disable(st->reg);
> > 353
> > 354 return ret;
> > 355 }
> >
> >
> > My initial thought was to just add:
> >
> > if (st->ext_ref && st->reg)
> > regulator_disable(st_reg);
> >
> > ..however it may be just that the code in line 302 should be aborting earlier
> > when the st->reg is set to NULL.
I think a slightly better option would be to finalize the conversion
to devm_ functions here.
That way, we can cleanup/reduce some of the code.
I do remember seeing that 'st->ext_ref' is being checked for some
regulator_xxx() function calls, and that isn't pretty.
But 'st->ext_ref' is zero when 'st->reg' is NULL.
So, this is a bit of a false-positive (I think).
I'll do the cleanup/conversion and then this should go away of coverity's radar.
Thanks
Alex
> >
> > Colin
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: iio: adc: ad7298: rework external ref setup & remove platform data
2020-11-27 9:24 ` FW: iio: adc: ad7298: rework external ref setup & remove platform data Alexandru Ardelean
@ 2020-11-27 12:15 ` Jonathan Cameron
0 siblings, 0 replies; 2+ messages in thread
From: Jonathan Cameron @ 2020-11-27 12:15 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: Alexandru Ardelean, colin.king, linux-iio
On Fri, 27 Nov 2020 11:24:16 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> On Fri, Nov 27, 2020 at 8:24 AM Ardelean, Alexandru
> <alexandru.Ardelean@analog.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Colin Ian King <colin.king@canonical.com>
> > > Sent: Wednesday, November 25, 2020 4:37 PM
> > > To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>; Jonathan
> > > Cameron <Jonathan.Cameron@huawei.com>
> > > Subject: re: iio: adc: ad7298: rework external ref setup & remove platform data
> > >
> > > [External]
> > >
> > > Hi there,
> > >
> > > Static analysis with Coverity on Linux-next today found a potential null pointer
> > > dereference. I believe it was triggered by the following commit:
> > >
> > > commit 28963f2f6b46d75bda8fed15bd5ce9923427a40d
> > > Author: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > > Date: Thu Oct 1 17:10:48 2020 +0300
> > >
> > > iio: adc: ad7298: rework external ref setup & remove platform data
> > >
> > > ..however the issue may have existed before now and this change triggered the
> > > bug being found.
> > >
> > > The analysis is as follows:
> > >
> > >
> > > 282 static int ad7298_probe(struct spi_device *spi)
> > > 283 {
> > > 284 struct ad7298_state *st;
> > > 285 struct iio_dev *indio_dev;
> > > 286 int ret;
> > > 287
> > > 288 indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > >
> > > 1. Condition indio_dev == NULL, taking false branch.
> > >
> > > 289 if (indio_dev == NULL)
> > > 290 return -ENOMEM;
> > > 291
> > > 292 st = iio_priv(indio_dev);
> > > 293
> > > 294 st->reg = devm_regulator_get_optional(&spi->dev, "vref");
> > >
> > > 2. Condition !IS_ERR(st->reg), taking false branch.
> > >
> > > 295 if (!IS_ERR(st->reg)) {
> > > 296 st->ext_ref = AD7298_EXTREF;
> > > 297 } else {
> > > 298 ret = PTR_ERR(st->reg);
> > >
> > > 3. Condition ret != -19, taking false branch.
> > >
> > > 299 if (ret != -ENODEV)
> > > 300 return ret;
> > > 301
> > > 302 st->reg = NULL;
> > > 303 }
> > > 304
> > >
> > > 4. Condition st->reg, taking false branch.
> > > 5. var_compare_op: Comparing st->reg to null implies that st->reg might be
> > > null.
> > >
> > > 305 if (st->reg) {
> > > 306 ret = regulator_enable(st->reg);
> > > 307 if (ret)
> > > 308 return ret;
> > > 309 }
> > > 310
> > > 311 spi_set_drvdata(spi, indio_dev);
> > > 312
> > > 313 st->spi = spi;
> > > 314
> > > 315 indio_dev->name = spi_get_device_id(spi)->name;
> > > 316 indio_dev->modes = INDIO_DIRECT_MODE;
> > > 317 indio_dev->channels = ad7298_channels;
> > > 318 indio_dev->num_channels = ARRAY_SIZE(ad7298_channels);
> > > 319 indio_dev->info = &ad7298_info;
> > > 320
> > > 321 /* Setup default message */
> > > 322
> > > 323 st->scan_single_xfer[0].tx_buf = &st->tx_buf[0];
> > > 324 st->scan_single_xfer[0].len = 2;
> > > 325 st->scan_single_xfer[0].cs_change = 1;
> > > 326 st->scan_single_xfer[1].tx_buf = &st->tx_buf[1];
> > > 327 st->scan_single_xfer[1].len = 2;
> > > 328 st->scan_single_xfer[1].cs_change = 1;
> > > 329 st->scan_single_xfer[2].rx_buf = &st->rx_buf[0];
> > > 330 st->scan_single_xfer[2].len = 2;
> > > 331
> > > 332 spi_message_init(&st->scan_single_msg);
> > > 333 spi_message_add_tail(&st->scan_single_xfer[0],
> > > &st->scan_single_msg);
> > > 334 spi_message_add_tail(&st->scan_single_xfer[1],
> > > &st->scan_single_msg);
> > > 335 spi_message_add_tail(&st->scan_single_xfer[2],
> > > &st->scan_single_msg);
> > > 336
> > > 337 ret = iio_triggered_buffer_setup(indio_dev, NULL,
> > > 338 &ad7298_trigger_handler, NULL);
> > >
> > > 6. Condition ret, taking true branch.
> > >
> > > 339 if (ret)
> > >
> > > 7. Jumping to label error_disable_reg.
> > >
> > > 340 goto error_disable_reg;
> > > 341
> > > 342 ret = iio_device_register(indio_dev);
> > > 343 if (ret)
> > > 344 goto error_cleanup_ring;
> > > 345
> > > 346 return 0;
> > > 347
> > > 348 error_cleanup_ring:
> > > 349 iio_triggered_buffer_cleanup(indio_dev);
> > > 350 error_disable_reg:
> > >
> > > 8. Condition st->ext_ref, taking true branch.
> > >
> > > 351 if (st->ext_ref)
> > >
> > > Dereference after null check (FORWARD_NULL)
> > > 9. var_deref_model: Passing null pointer st->reg to regulator_disable, which
> > > dereferences it.
> > >
> > > 352 regulator_disable(st->reg);
> > > 353
> > > 354 return ret;
> > > 355 }
> > >
> > >
> > > My initial thought was to just add:
> > >
> > > if (st->ext_ref && st->reg)
> > > regulator_disable(st_reg);
> > >
> > > ..however it may be just that the code in line 302 should be aborting earlier
> > > when the st->reg is set to NULL.
>
> I think a slightly better option would be to finalize the conversion
> to devm_ functions here.
> That way, we can cleanup/reduce some of the code.
> I do remember seeing that 'st->ext_ref' is being checked for some
> regulator_xxx() function calls, and that isn't pretty.
> But 'st->ext_ref' is zero when 'st->reg' is NULL.
> So, this is a bit of a false-positive (I think).
Agreed it's a false positive though would make sense to check st->reg rather than
st->ext_ref here.
>
> I'll do the cleanup/conversion and then this should go away of coverity's radar.
>
> Thanks
> Alex
>
> > >
> > > Colin
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-11-27 12:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <442ef85b-47a0-09a9-7f6a-882f1817e51a@canonical.com>
[not found] ` <DM6PR03MB4411E6C39D991B5C5AAC1F86F9F80@DM6PR03MB4411.namprd03.prod.outlook.com>
2020-11-27 9:24 ` FW: iio: adc: ad7298: rework external ref setup & remove platform data Alexandru Ardelean
2020-11-27 12:15 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).