Message-ID: <20221114101318epcms2p7fb5585f189a480717a80518ef1a8b8ba@epcms2p7>
Date:   Mon, 14 Nov 2022 19:13:18 +0900
From:   Bongsu Jeon <bongsu.jeon@...sung.com>
To:     Dmitry Vyukov <dvyukov@...gle.com>,
        Bongsu Jeon <bongsu.jeon@...sung.com>
CC:     "leon@...nel.org" <leon@...nel.org>,
        "krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "syzkaller@...glegroups.com" <syzkaller@...glegroups.com>
Subject: Re: [PATCH net-next v3] nfc: Allow to create multiple virtual nci
 devices

On Mon, Nov 14, 2022 at 6:54 PM Dmitry Vyukov<dvyukov@...gle.com> wrote:
> On Mon, 14 Nov 2022 at 00:32, Bongsu Jeon <bongsu.jeon@...sung.com> wrote:
> > > > > > > > On Sat, Nov 5, 2022 at 2:04 AM Dmitry Vyukov<dvyukov@...gle.com> wrote:
> > > > > > > > > The current virtual nci driver is great for testing and fuzzing.
> > > > > > > > > But it allows to create at most one "global" device which does not allow
> > > > > > > > > to run parallel tests and harms fuzzing isolation and reproducibility.
> > > > > > > > > Restructure the driver to allow creation of multiple independent devices.
> > > > > > > > > This should be backwards compatible for existing tests.
> > > > > > > >
> > > > > > > > I totally agree with you for parallel tests and good design.
> > > > > > > > Thanks for good idea.
> > > > > > > > But please check the abnormal situation.
> > > > > > > > for example virtual device app is closed(virtual_ncidev_close) first and then
> > > > > > > > virtual nci driver from nci app tries to call virtual_nci_send or virtual_nci_close.
> > > > > > > > (there would be problem in virtual_nci_send because of already destroyed mutex)
> > > > > > > > Before this patch, this driver used virtual_ncidev_mode state and nci_mutex that isn't destroyed.
> > > > > > >
> > > > > > > I assumed nci core must stop calling into a driver at some point
> > > > > > > during the driver destruction. And I assumed that point is return from
> > > > > > > nci_unregister_device(). Basically when nci_unregister_device()
> > > > > > > returns, no new calls into the driver must be made. Calling into a
> > > > > > > driver after nci_unregister_device() looks like a bug in nci core.
> > > > > > >
> > > > > > > If this is not true, how do real drivers handle this? They don't use
> > > > > > > global vars. So they should either have the same use-after-free bugs
> > > > > > > you described, or they handle shutdown differently. We just need to do
> > > > > > > the same thing that real drivers do.
> > > > > > >
> > > > > > > As far as I see they are doing the same what I did in this patch:
> > > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/fdp/i2c.c#L343
> > > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/usb.c#L354
> > > > > > >
> > > > > > > They call nci_unregister_device() and then free all resources:
> > > > > > > https://elixir.bootlin.com/linux/v6.1-rc4/source/drivers/nfc/nfcmrvl/main.c#L186
> > > > > > >
> > > > > > > What am I missing here?
> > > > > >
> > > > > > I'm not sure but I think they are little different.
> > > > > > nfcmrvl uses usb_driver's disconnect callback function and fdp's i2c uses i2c_driver's remove callback function for unregister_device.
> > > > > > But virtual_ncidev just uses file operation(close function) not related to driver.
> > > > > > so Nci simulation App can call close function at any time.
> > > > > > If Scheduler interrupts the nci core code right after calling virtual_nci_send and then
> > > > > > other process or thread calls virtual_nci_dev's close function,
> > > > > > we need to handle this problem in virtual nci driver.
> > > > >
> > > > > Won't the same issue happen if nci send callback is concurrent with
> > > > > USB/I2C driver disconnect?
> > > > >
> > > > > I mean something internal to the USB subsystem cannot affect what nci
> > > > > subsystem is doing, unless the USB driver calls into nci and somehow
> > > > > notifies it that it's about to destroy the driver.
> > > > >
> > > > > Is there anything USB/I2C drivers are doing besides calling
> > > > > nci_unregister_device() to ensure that there are no pending nci send
> > > > > calls? If yes, then we should do the same in the virtual driver. If
> > > > > not, then all other drivers are the subject to the same use-after-free
> > > > > bug.
> > > > >
> > > > > But I assumed that nci_unregister_device() ensures that there are no
> > > > > in-flight send calls and no future send calls will be issued after the
> > > > > function returns.
> > > >
> > > > Ok, I understand your mention. you mean that nci_unregister_device should prevent
> > > > the issue using dev lock or other way. right?
> > >
> > > Yes.
> > >
> > > > It would be better to handle the issue in nci core if there is.
> > >
> > > And yes.
> > >
> > > Krzysztof, can you confirm this is the case (nci core won't call
> > > ops->send callback after nci_unregister_device() returns)?
> >
> > I think we can add this to selftest to verify nci core.
> 
> I am not sure how the test for that particular scenario should look
> like. It's only possible with concurrent syscalls, right? After
> nci_unregister_device() returns and the virtual device fd is closed,
> it's not possible to trigger the send callback, right?
> 

As you think, we can't control concurrent timing so that it will be hard to
test exactly same scenario that i asked first.
I just wanted to see the simple scenario testcase with single thread.
I thought following simple sequence.

1. virtualDevFd = open() // for virtual device 
2. enable and open nci dev and connect NFC socket with virtualDevFd using NFC/NCI System Call
3. close virtualDevFd.( nci_unregister_device )
4. send test data using connected NFC socket.
=> if socket write operation failed and there were no issues in kernel, then it works properly.

> 
> > > > > > > > > Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
> > > > > > > > > Cc: Bongsu Jeon <bongsu.jeon@...sung.com>
> > > > > > > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> > > > > > > > > Cc: netdev@...r.kernel.org
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > > Changes in v3:
> > > > > > > > >  - free vdev in virtual_ncidev_close()
> > > > > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > >  - check return value of skb_clone()
> > > > > > > > >  - rebase onto currnet net-next
> > > > > > > > > ---
> > > > > > > > >  drivers/nfc/virtual_ncidev.c | 147 +++++++++++++++++------------------
> > > > > > > > >  1 file changed, 71 insertions(+), 76 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/nfc/virtual_ncidev.c b/drivers/nfc/virtual_ncidev.c
> > > > > > > > > index 85c06dbb2c449..bb76c7c7cc822 100644
> > > > > > > > > --- a/drivers/nfc/virtual_ncidev.c
> > > > > > > > > +++ b/drivers/nfc/virtual_ncidev.c
> > > > > > > > > @@ -13,12 +13,6 @@
> > > > > > > > >
> > > > > > > > >  static int virtual_nci_send(struct nci_dev *ndev, struct sk_buff *skb)
> > > > > > > > >  {
> > > > > > > > > -     mutex_lock(&nci_mutex);
> > > > > > > > > -     if (state != virtual_ncidev_enabled) {
> > > > > > > > > -             mutex_unlock(&nci_mutex);
> > > > > > > > > +     struct virtual_nci_dev *vdev = nci_get_drvdata(ndev);
> > > > > > > > > +
> > > > > > > > > +     mutex_lock(&vdev->mtx);
> > > > > > > >
> > > > > > > >   I think this vdev and vdev->mtx are already destroyed so that it would be problem.
> > > > > > > >
> > > > > > > > > +     if (vdev->send_buff) {
> > > > > > > > > +             mutex_unlock(&vdev->mtx);
> > > > > > > > >               kfree_skb(skb);
> > > > > > > > > -             return 0;
> > > > > > > > > +             return -1;
> > > > > > > > >       }
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >  static int virtual_ncidev_close(struct inode *inode, struct file *file)
> > > > > > > > >  {
> > > > > > > > > -     mutex_lock(&nci_mutex);
> > > > > > > > > -
> > > > > > > > > -     if (state == virtual_ncidev_enabled) {
> > > > > > > > > -             state = virtual_ncidev_disabling;
> > > > > > > > > -             mutex_unlock(&nci_mutex);
> > > > > > > > > +     struct virtual_nci_dev *vdev = file->private_data;
> > > > > > > > >
> > > > > > > > > -             nci_unregister_device(ndev);
> > > > > > > > > -             nci_free_device(ndev);
> > > > > > > > > -
> > > > > > > > > -             mutex_lock(&nci_mutex);
> > > > > > > > > -     }
> > > > > > > > > -
> > > > > > > > > -     state = virtual_ncidev_disabled;
> > > > > > > > > -     mutex_unlock(&nci_mutex);
> > > > > > > > > +     nci_unregister_device(vdev->ndev);
> > > > > > > > > +     nci_free_device(vdev->ndev);
> > > > > > > > > +     mutex_destroy(&vdev->mtx);
> > > > > > > > > +     kfree(vdev);
> > > > > > > > >
> > > > > > > > >       return 0;
> > > > > > > > >  }
> > > > >

Powered by blists - more mailing lists