lkml.org 
[lkml]   [2019]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: spi: Add call to spi_slave_abort() function when spidev driver is released
From
Date
On 26/09/2019 11:14, Lukasz Majewski wrote:
> 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.

Cool, thanks for the update.

Colin
>
>>
>> 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:16    [W:0.048 / U:0.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site