Re: spi: Add call to spi_slave_abort() function when spidev driver is released

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 2866 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]