netdev - Re: ibmvnic: Race condition in remove callback

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: <b725079b34031595887b019d1d2f6fc7@imap.linux.ibm.com>
Date:   Tue, 19 Jan 2021 10:14:25 -0800
From:   Dany Madden <drt@...ux.ibm.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Lijun Pan <ljp@...ux.ibm.com>,
        Sukadev Bhattiprolu <sukadev@...ux.ibm.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Juliet Kim <julietk@...ux.vnet.ibm.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        linuxppc-dev@...ts.ozlabs.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel@...gutronix.de
Subject: Re: ibmvnic: Race condition in remove callback

On 2021-01-17 02:12, Uwe Kleine-König wrote:
> Hello,
> 
> while working on some cleanup I stumbled over a problem in the 
> ibmvnic's
> remove callback. Since commit
> 
>         7d7195a026ba ("ibmvnic: Do not process device remove during
> device reset")
> 
> there is the following code in the remove callback:
> 
>         static int ibmvnic_remove(struct vio_dev *dev)
>         {
>                 ...
>                 spin_lock_irqsave(&adapter->state_lock, flags);
>                 if (test_bit(0, &adapter->resetting)) {
>                         spin_unlock_irqrestore(&adapter->state_lock, 
> flags);
>                         return -EBUSY;
>                 }
> 
>                 adapter->state = VNIC_REMOVING;
>                 spin_unlock_irqrestore(&adapter->state_lock, flags);
> 
>                 flush_work(&adapter->ibmvnic_reset);
>                 flush_delayed_work(&adapter->ibmvnic_delayed_reset);
>                 ...
>         }
> 
> Unfortunately returning -EBUSY doesn't work as intended. That's because
> the return value of this function is ignored[1] and the device is
> considered unbound by the device core (shortly) after ibmvnic_remove()
> returns.

Oh! I was not aware of this. In our code review, a question on whether 
or not device reset should have a higher precedence over device remove 
was raised before. So, now it is clear that this driver has to take care 
of remove over reset.

> 
> While looking into fixing that I noticed a worse problem:
> 
> If ibmvnic_reset() (e.g. called by the tx_timeout callback) calls
> schedule_work(&adapter->ibmvnic_reset); just after the work queue is
> flushed above the problem that 7d7195a026ba intends to fix will trigger
> resulting in a use-after-free.

It was proposed that when coming into ibmvnic_remove() we lock down the 
workqueue to prevent future access, flush, cleanup, then unregister the 
device. Your thought on this?

> 
> Also ibmvnic_reset() checks for adapter->state without holding the lock
> which might be racy, too.
> 
Suka started addressing consistent locking with this patch series:
https://lists.openwall.net/netdev/2021/01/08/89

He is reworking this.

> Best regards
> Uwe

Thank you for taking the time to review this driver, Uwe. This is very 
helpful for us.

Best Regards,
Dany

> 
> [1] vio_bus_remove (in arch/powerpc/platforms/pseries/vio.c) records 
> the
>     return value and passes it on. But the driver core doesn't care for
>     the return value (see __device_release_driver() in 
> drivers/base/dd.c
>     calling dev->bus->remove()).