lkml.org 
[lkml]   [2019]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: spi: Add call to spi_slave_abort() function when spidev driver is released
Hi Colin,

> Hi,
>
> Static analysis with Coverity has detected an potential dereference
> of a free'd object with commit:
>
> commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> Author: Lukasz Majewski <lukma@denx.de>
> Date: Wed Sep 25 11:11:42 2019 +0200
>
> spi: Add call to spi_slave_abort() function when spidev driver is
> released
>
> In spidev_release() in drivers/spi/spidev.c the analysis is as
> follows:
>
> 600static int spidev_release(struct inode *inode, struct file *filp)
> 601{
> 602 struct spidev_data *spidev;
> 603
> 604 mutex_lock(&device_list_lock);
>
> 1. alias: Assigning: spidev = filp->private_data. Now both point to
> the same storage.
>
> 605 spidev = filp->private_data;
> 606 filp->private_data = NULL;
> 607
> 608 /* last close? */
> 609 spidev->users--;
>
> 2. Condition !spidev->users, taking true branch.
>
> 610 if (!spidev->users) {
> 611 int dofree;
> 612
> 613 kfree(spidev->tx_buffer);
> 614 spidev->tx_buffer = NULL;
> 615
> 616 kfree(spidev->rx_buffer);
> 617 spidev->rx_buffer = NULL;
> 618
> 619 spin_lock_irq(&spidev->spi_lock);
>
> 3. Condition spidev->spi, taking false branch.
>
> 620 if (spidev->spi)
> 621 spidev->speed_hz =
> spidev->spi->max_speed_hz; 622
> 623 /* ... after we unbound from the underlying
> device? */
>
> 4. Condition spidev->spi == NULL, taking true branch.
>
> 624 dofree = (spidev->spi == NULL);
> 625 spin_unlock_irq(&spidev->spi_lock);
> 626
>
> 5. Condition dofree, taking true branch.
>
> 627 if (dofree)
>
> 6. freed_arg: kfree frees spidev.
>
> 628 kfree(spidev);
> 629 }
> 630#ifdef CONFIG_SPI_SLAVE
>
> CID 89726 (#1 of 1): Read from pointer after free (USE_AFTER_FREE)
> 7. deref_after_free: Dereferencing freed pointer spidev.
>
> 631 spi_slave_abort(spidev->spi);
> 632#endif
> 633 mutex_unlock(&device_list_lock);
> 634
> 635 return 0;
> 636}
>
> The call to spi_slave_abort() on spidev is reading an earlier kfree'd
> spidev.

Thanks for spotting this issue - indeed there is a possibility to use
spidev after being kfree'd.

However, Geert (CC'ed) had some questions about placement of this
function call, so I will wait with providing fix until he replies.

>
> Colin
>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2019-09-26 12:15    [W:1.209 / U:0.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site