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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190926121438.655f1f10@jawa>
Date:   Thu, 26 Sep 2019 12:14:38 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Colin Ian King <colin.king@...onical.com>
Cc:     Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: 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@...x.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@...x.de

Content of type "application/pgp-signature" skipped