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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Lukasz,

On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <lukma@xxxxxxx> wrote:
> > Static analysis with Coverity has detected an potential dereference
> > of a free'd object with commit:
> >
> > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > Author: Lukasz Majewski <lukma@xxxxxxx>
> > 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.

Worse, this makes me realize spidev->spi may be a NULL pointer, which
will be dereferenced by spi_slave_abort(), so caching it before the
call to kfree() won't work.

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

Seems like this needs more thought...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds