netdev - Re: [PATCH net-next v3] nfc: Allow to create multiple virtual nci devices

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]

Date:   Mon, 7 Nov 2022 10:38:22 -0800
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     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 Sun, 6 Nov 2022 at 18:46, 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?


> > 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;
> >  }