Hi,
Static analysis with Coverity has detected an potential dereference of a
free'd object with commit:
commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
Author: Lukasz Majewski <[email protected]>
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.
Colin
Hi Colin,
> Hi,
>
> Static analysis with Coverity has detected an potential dereference
> of a free'd object with commit:
>
> commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> Author: Lukasz Majewski <[email protected]>
> 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: [email protected]
Hi Lukasz,
On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <[email protected]> wrote:
> > Static analysis with Coverity has detected an potential dereference
> > of a free'd object with commit:
> >
> > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > Author: Lukasz Majewski <[email protected]>
> > 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 -- [email protected]
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
Hi Geert,
> Hi Lukasz,
>
> On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <[email protected]>
> wrote:
> > > Static analysis with Coverity has detected an potential
> > > dereference of a free'd object with commit:
> > >
> > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > Author: Lukasz Majewski <[email protected]>
> > > 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.
>
The patch as it is now can be fixed as follows:
static int spidev_release(struct inode *inode, struct file *filp)
{
struct spidev_data *spidev;
mutex_lock(&device_list_lock);
spidev = filp->private_data;
filp->private_data = NULL;
#ifdef CONFIG_SPI_SLAVE
if (spidev->spi)
spi_slave_abort(spidev->spi);
#endif
/* last close? */
spidev->users--;
if (!spidev->users) {
int dofree;
/* free buffers */
spin_lock_irq(&spidev->spi_lock);
if (spidev->spi)
spidev->speed_hz = spidev->spi->max_speed_hz;
/* ... after we unbound from the underlying device? */
//
// [*]
//
dofree = (spidev->spi == NULL);
spin_unlock_irq(&spidev->spi_lock);
if (dofree)
kfree(spidev);
}
mutex_unlock(&device_list_lock);
return 0;
}
The question is if we shall call the spi_slave_abort() when cleaning up
spi after releasing last reference, or each time release callback is
called ?
> > 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...
Could you be more specific?
Do you mean to move the spi_slave_abort() call just before dofree
evaluation ? ([*]).
>
> Gr{oetje,eeting}s,
>
> Geert
>
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: [email protected]
Hi Lukasz,
On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <[email protected]> wrote:
> > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <[email protected]>
> > wrote:
> > > > Static analysis with Coverity has detected an potential
> > > > dereference of a free'd object with commit:
> > > >
> > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > Author: Lukasz Majewski <[email protected]>
> > > > Date: Wed Sep 25 11:11:42 2019 +0200
> > > >
> > > > spi: Add call to spi_slave_abort() function when spidev
> > > > driver is released
> > > > 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.
>
> The patch as it is now can be fixed as follows:
>
> static int spidev_release(struct inode *inode, struct file *filp)
> {
> struct spidev_data *spidev;
>
> mutex_lock(&device_list_lock);
> spidev = filp->private_data;
> filp->private_data = NULL;
>
> #ifdef CONFIG_SPI_SLAVE
> if (spidev->spi)
> spi_slave_abort(spidev->spi);
> #endif
>
> /* last close? */
> spidev->users--;
> if (!spidev->users) {
> int dofree;
>
> /* free buffers */
>
> spin_lock_irq(&spidev->spi_lock);
> if (spidev->spi)
> spidev->speed_hz = spidev->spi->max_speed_hz;
>
> /* ... after we unbound from the underlying device? */
> //
> // [*]
> //
> dofree = (spidev->spi == NULL);
> spin_unlock_irq(&spidev->spi_lock);
>
> if (dofree)
> kfree(spidev);
> }
>
> mutex_unlock(&device_list_lock);
>
> return 0;
> }
>
> The question is if we shall call the spi_slave_abort() when cleaning up
> spi after releasing last reference, or each time release callback is
> called ?
TBH, I don't know. Is it realistic that there are multiple opens?
> > > 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...
>
> Could you be more specific?
>
> Do you mean to move the spi_slave_abort() call just before dofree
> evaluation ? ([*]).
That means the abort is called only for the last user.
And only if the underlying device still exists. Which means that if it has
disappeared (how can that happen? spidev unbind?), the slave was never
aborted. Non-spidev slaves can do the abort in their .remove() callbacks
(at least my two sample slave drivers do).
So probably we need some explicit slave abort in the unbind case too?
The more I think about it, the more things I see that can go wrong...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Hi Geert,
> Hi Lukasz,
>
> On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <[email protected]> wrote:
> > > On Thu, Sep 26, 2019 at 12:14 PM Lukasz Majewski <[email protected]>
> > > wrote:
> > > > > Static analysis with Coverity has detected an potential
> > > > > dereference of a free'd object with commit:
> > > > >
> > > > > commit 9f918a728cf86b2757b6a7025e1f46824bfe3155
> > > > > Author: Lukasz Majewski <[email protected]>
> > > > > Date: Wed Sep 25 11:11:42 2019 +0200
> > > > >
> > > > > spi: Add call to spi_slave_abort() function when spidev
> > > > > driver is released
>
> > > > > 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.
> >
> > The patch as it is now can be fixed as follows:
> >
> > static int spidev_release(struct inode *inode, struct file *filp)
> > {
> > struct spidev_data *spidev;
> >
> > mutex_lock(&device_list_lock);
> > spidev = filp->private_data;
> > filp->private_data = NULL;
> >
> > #ifdef CONFIG_SPI_SLAVE
> > if (spidev->spi)
> > spi_slave_abort(spidev->spi);
> > #endif
> >
> > /* last close? */
> > spidev->users--;
> > if (!spidev->users) {
> > int dofree;
> >
> > /* free buffers */
> >
> > spin_lock_irq(&spidev->spi_lock);
> > if (spidev->spi)
> > spidev->speed_hz =
> > spidev->spi->max_speed_hz;
> >
> > /* ... after we unbound from the underlying device?
> > */ //
> > // [*]
> > //
> > dofree = (spidev->spi == NULL);
> > spin_unlock_irq(&spidev->spi_lock);
> >
> > if (dofree)
> > kfree(spidev);
> > }
> >
> > mutex_unlock(&device_list_lock);
> >
> > return 0;
> > }
> >
> > The question is if we shall call the spi_slave_abort() when
> > cleaning up spi after releasing last reference, or each time
> > release callback is called ?
>
> TBH, I don't know. Is it realistic that there are multiple opens?
I'm using on my setup only one test program to use /dev/spidevX.Y and
/dev/spidevA.B (loopback with wired connection).
However, you also shall be able to connect via ssh and run the same
setup in parallel...
>
> > > > 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...
> >
> > Could you be more specific?
> >
> > Do you mean to move the spi_slave_abort() call just before dofree
> > evaluation ? ([*]).
>
> That means the abort is called only for the last user.
> And only if the underlying device still exists. Which means that if
> it has disappeared (how can that happen? spidev unbind?),
In my case, I just disconnect some SPI signals and the test program
just hangs. I do need to ctrl+c to stop it (or use timeout).
From my debugging the .release callback is called each time the program
is aborted (either with ctrl+c or timeout).
> the slave
> was never aborted. Non-spidev slaves can do the abort in their
> .remove() callbacks (at least my two sample slave drivers do).
> So probably we need some explicit slave abort in the unbind case too?
As I've described above - after "introducing" distortion to SPI I need
to explicitly exit the hung test program with ctrl+c.
>
> The more I think about it, the more things I see that can go wrong...
But for now we don't have any way to recover the slave after corruption
on SPI transmission.
>
> Gr{oetje,eeting}s,
>
> Geert
>
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: [email protected]
On Thu, Sep 26, 2019 at 04:06:45PM +0200, Lukasz Majewski wrote:
> > On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <[email protected]> wrote:
> > > The question is if we shall call the spi_slave_abort() when
> > > cleaning up spi after releasing last reference, or each time
> > > release callback is called ?
> > TBH, I don't know. Is it realistic that there are multiple opens?
> I'm using on my setup only one test program to use /dev/spidevX.Y and
> /dev/spidevA.B (loopback with wired connection).
> However, you also shall be able to connect via ssh and run the same
> setup in parallel...
It doesn't seem entirely realistic, but I can imagine cases like
fork()/exec() where we end up with two copies of the file open
but end up immediately closing one.
> > That means the abort is called only for the last user.
> > And only if the underlying device still exists. Which means that if
> > it has disappeared (how can that happen? spidev unbind?),
> In my case, I just disconnect some SPI signals and the test program
> just hangs. I do need to ctrl+c to stop it (or use timeout).
> From my debugging the .release callback is called each time the program
> is aborted (either with ctrl+c or timeout).
Should be on file close IIRC.
Hi Mark, Geert,
> On Thu, Sep 26, 2019 at 04:06:45PM +0200, Lukasz Majewski wrote:
> > > On Thu, Sep 26, 2019 at 2:49 PM Lukasz Majewski <[email protected]>
> > > wrote:
>
> > > > The question is if we shall call the spi_slave_abort() when
> > > > cleaning up spi after releasing last reference, or each time
> > > > release callback is called ?
>
> > > TBH, I don't know. Is it realistic that there are multiple
> > > opens?
>
> > I'm using on my setup only one test program to use /dev/spidevX.Y
> > and /dev/spidevA.B (loopback with wired connection).
>
> > However, you also shall be able to connect via ssh and run the same
> > setup in parallel...
>
> It doesn't seem entirely realistic, but I can imagine cases like
> fork()/exec() where we end up with two copies of the file open
> but end up immediately closing one.
>
> > > That means the abort is called only for the last user.
> > > And only if the underlying device still exists. Which means that
> > > if it has disappeared (how can that happen? spidev unbind?),
>
> > In my case, I just disconnect some SPI signals and the test program
> > just hangs. I do need to ctrl+c to stop it (or use timeout).
>
> > From my debugging the .release callback is called each time the
> > program is aborted (either with ctrl+c or timeout).
>
> Should be on file close IIRC.
Any ideas on how to solve this issue?
Maybe, it would be sufficient for now to move the spi_slave_abort() in
spi_release() before we decrease (spidev->users--) the use count?
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: [email protected]
On Fri, Sep 27, 2019 at 12:38:49AM +0200, Lukasz Majewski wrote:
> Maybe, it would be sufficient for now to move the spi_slave_abort() in
> spi_release() before we decrease (spidev->users--) the use count?
I think that should be OK, or possibly safer to do it at the start of
the if (!spidev->users) section to avoid problems with fork()/exec()
combinations. It'll need an if (spidev->spi) check as well, if we
manage to hit that we've pretty much lost anyway though and I'm not sure
I see a route to sensible error handling.