[PATCH net v4] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs

[PATCH net v4] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs

[Duoming Zhou]

There are destructive operations such as nfcmrvl_fw_dnld_abort and
gpio_free in nfcmrvl_nci_unregister_dev. The resources such as firmware,
gpio and so on could be destructed while the upper layer functions such as
nfcmrvl_fw_dnld_start and nfcmrvl_nci_recv_frame is executing, which leads
to double-free, use-after-free and null-ptr-deref bugs.

There are three situations that could lead to double-free bugs.

The first situation is shown below:

   (Thread 1)                 |      (Thread 2)
nfcmrvl_fw_dnld_start         |
 ...                          |  nfcmrvl_nci_unregister_dev
 release_firmware()           |   nfcmrvl_fw_dnld_abort
  kfree(fw) //(1)             |    fw_dnld_over
                              |     release_firmware
  ...                         |      kfree(fw) //(2)
                              |     ...

The second situation is shown below:

   (Thread 1)                 |      (Thread 2)
nfcmrvl_fw_dnld_start         |
 ...                          |
 mod_timer                    |
 (wait a time)                |
 fw_dnld_timeout              |  nfcmrvl_nci_unregister_dev
   fw_dnld_over               |   nfcmrvl_fw_dnld_abort
    release_firmware          |    fw_dnld_over
     kfree(fw) //(1)          |     release_firmware
     ...                      |      kfree(fw) //(2)

The third situation is shown below:

       (Thread 1)               |       (Thread 2)
nfcmrvl_nci_recv_frame          |
 if(..->fw_download_in_progress)|
  nfcmrvl_fw_dnld_recv_frame    |
   queue_work                   |
                                |
fw_dnld_rx_work                 | nfcmrvl_nci_unregister_dev
 fw_dnld_over                   |  nfcmrvl_fw_dnld_abort
  release_firmware              |   fw_dnld_over
   kfree(fw) //(1)              |    release_firmware
                                |     kfree(fw) //(2)

The firmware struct is deallocated in position (1) and deallocated
in position (2) again.

The crash trace triggered by POC is like below:

[  122.640457] BUG: KASAN: double-free or invalid-free in fw_dnld_over+0x28/0xf0
[  122.640457] Call Trace:
[  122.640457]  <TASK>
[  122.640457]  kfree+0xb0/0x330
[  122.640457]  fw_dnld_over+0x28/0xf0
[  122.640457]  nfcmrvl_nci_unregister_dev+0x61/0x70
[  122.640457]  nci_uart_tty_close+0x87/0xd0
[  122.640457]  tty_ldisc_kill+0x3e/0x80
[  122.640457]  tty_ldisc_hangup+0x1b2/0x2c0
[  122.640457]  __tty_hangup.part.0+0x316/0x520
[  122.640457]  tty_release+0x200/0x670
[  122.640457]  __fput+0x110/0x410
[  122.640457]  task_work_run+0x86/0xd0
[  122.640457]  exit_to_user_mode_prepare+0x1aa/0x1b0
[  122.640457]  syscall_exit_to_user_mode+0x19/0x50
[  122.640457]  do_syscall_64+0x48/0x90
[  122.640457]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[  122.640457] RIP: 0033:0x7f68433f6beb

What's more, there are also use-after-free and null-ptr-deref bugs
in nfcmrvl_fw_dnld_start. If we deallocate firmware struct, gpio or
set null to the members of priv->fw_dnld in nfcmrvl_nci_unregister_dev,
then, we dereference firmware, gpio or the members of priv->fw_dnld in
nfcmrvl_fw_dnld_start, the UAF or NPD bugs will happen.

This patch reorders destructive operations after nci_unregister_device
and adds bool variable protected by device_lock to synchronize between
cleanup routine and firmware download routine. The process is shown below.

       (Thread 1)               |       (Thread 2)
nfcmrvl_nci_unregister_dev      |
  nci_unregister_device         |
    nfc_unregister_device       | nfc_fw_download
      device_lock()             |
      ...                       |
      nfc_download = false;     |   ...
      device_unlock()           |
  ...                           |   device_lock()
  (destructive operations)      |   if(.. || !nfc_download)
                                |     goto error;
                                | error:
                                |   device_unlock()

If the device is detaching, the download function will goto error.
If the download function is executing, nci_unregister_device will
wait until download function is finished.

Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v4:
  - Change bool variable nfc_download to static.

 drivers/nfc/nfcmrvl/main.c | 2 +-
 net/nfc/core.c             | 6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 2fcf545012b..1a5284de434 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -183,6 +183,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 {
 	struct nci_dev *ndev = priv->ndev;
 
+	nci_unregister_device(ndev);
 	if (priv->ndev->nfc_dev->fw_download_in_progress)
 		nfcmrvl_fw_dnld_abort(priv);
 
@@ -191,7 +192,6 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 	if (gpio_is_valid(priv->config.reset_n_io))
 		gpio_free(priv->config.reset_n_io);
 
-	nci_unregister_device(ndev);
 	nci_free_device(ndev);
 	kfree(priv);
 }
diff --git a/net/nfc/core.c b/net/nfc/core.c
index dc7a2404efd..1d91334ee86 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -25,6 +25,8 @@
 #define NFC_CHECK_PRES_FREQ_MS	2000
 
 int nfc_devlist_generation;
+/* nfc_download: used to judge whether nfc firmware download could start */
+static bool nfc_download;
 DEFINE_MUTEX(nfc_devlist_mutex);
 
 /* NFC device ID bitmap */
@@ -38,7 +40,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
 
 	device_lock(&dev->dev);
 
-	if (!device_is_registered(&dev->dev)) {
+	if (!device_is_registered(&dev->dev) || !nfc_download) {
 		rc = -ENODEV;
 		goto error;
 	}
@@ -1134,6 +1136,7 @@ int nfc_register_device(struct nfc_dev *dev)
 			dev->rfkill = NULL;
 		}
 	}
+	nfc_download = true;
 	device_unlock(&dev->dev);
 
 	rc = nfc_genl_device_added(dev);
@@ -1166,6 +1169,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
 		rfkill_unregister(dev->rfkill);
 		rfkill_destroy(dev->rfkill);
 	}
+	nfc_download = false;
 	device_unlock(&dev->dev);
 
 	if (dev->ops->check_presence) {
-- 
2.17.1

Re: [PATCH net v4] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs

[Jakub Kicinski]

On Wed, 27 Apr 2022 09:14:38 +0800 Duoming Zhou wrote:
> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index dc7a2404efd..1d91334ee86 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -25,6 +25,8 @@
>  #define NFC_CHECK_PRES_FREQ_MS	2000
>  
>  int nfc_devlist_generation;
> +/* nfc_download: used to judge whether nfc firmware download could start */
> +static bool nfc_download;
>  DEFINE_MUTEX(nfc_devlist_mutex);
>  
>  /* NFC device ID bitmap */
> @@ -38,7 +40,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
>  
>  	device_lock(&dev->dev);
>  
> -	if (!device_is_registered(&dev->dev)) {
> +	if (!device_is_registered(&dev->dev) || !nfc_download) {
>  		rc = -ENODEV;
>  		goto error;
>  	}
> @@ -1134,6 +1136,7 @@ int nfc_register_device(struct nfc_dev *dev)
>  			dev->rfkill = NULL;
>  		}
>  	}
> +	nfc_download = true;
>  	device_unlock(&dev->dev);
>  
>  	rc = nfc_genl_device_added(dev);
> @@ -1166,6 +1169,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
>  		rfkill_unregister(dev->rfkill);
>  		rfkill_destroy(dev->rfkill);
>  	}
> +	nfc_download = false;
>  	device_unlock(&dev->dev);
>  
>  	if (dev->ops->check_presence) {

You can't use a single global variable, there can be many devices 
each with their own lock.

Paolo suggested adding a lock, if spin lock doesn't fit the bill
why not add a mutex?

Re: Re: [PATCH net v4] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs

[duoming]

Hello,

On Wed, 27 Apr 2022 17:45:48 -0700 Jakub Kicinski wrote:

> > diff --git a/net/nfc/core.c b/net/nfc/core.c
> > index dc7a2404efd..1d91334ee86 100644
> > --- a/net/nfc/core.c
> > +++ b/net/nfc/core.c
> > @@ -25,6 +25,8 @@
> >  #define NFC_CHECK_PRES_FREQ_MS	2000
> >  
> >  int nfc_devlist_generation;
> > +/* nfc_download: used to judge whether nfc firmware download could start */
> > +static bool nfc_download;
> >  DEFINE_MUTEX(nfc_devlist_mutex);
> >  
> >  /* NFC device ID bitmap */
> > @@ -38,7 +40,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
> >  
> >  	device_lock(&dev->dev);
> >  
> > -	if (!device_is_registered(&dev->dev)) {
> > +	if (!device_is_registered(&dev->dev) || !nfc_download) {
> >  		rc = -ENODEV;
> >  		goto error;
> >  	}
> > @@ -1134,6 +1136,7 @@ int nfc_register_device(struct nfc_dev *dev)
> >  			dev->rfkill = NULL;
> >  		}
> >  	}
> > +	nfc_download = true;
> >  	device_unlock(&dev->dev);
> >  
> >  	rc = nfc_genl_device_added(dev);
> > @@ -1166,6 +1169,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
> >  		rfkill_unregister(dev->rfkill);
> >  		rfkill_destroy(dev->rfkill);
> >  	}
> > +	nfc_download = false;
> >  	device_unlock(&dev->dev);
> >  
> >  	if (dev->ops->check_presence) {
> 
> You can't use a single global variable, there can be many devices 
> each with their own lock.
> 
> Paolo suggested adding a lock, if spin lock doesn't fit the bill
> why not add a mutex?

We could not use mutex either, because the release_firmware() is also called by fw_dnld_timeout()
which is a timer handler. If we use mutex lock in a timer handler, it will cause sleep in atomic bug.
The process is shown below:

nfcmrvl_fw_dnld_start
 ...              
 mod_timer 
 (wait a time)  
 fw_dnld_timeout
   fw_dnld_over 
    release_firmware       

I will change the single global variable to dev->dev_up flag, which is shown below:

diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 2fcf545012b..1a5284de434 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -183,6 +183,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
 {
        struct nci_dev *ndev = priv->ndev;

+       nci_unregister_device(ndev);
        if (priv->ndev->nfc_dev->fw_download_in_progress)
                nfcmrvl_fw_dnld_abort(priv);

@@ -191,7 +192,6 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
        if (gpio_is_valid(priv->config.reset_n_io))
                gpio_free(priv->config.reset_n_io);

-       nci_unregister_device(ndev);
        nci_free_device(ndev);
        kfree(priv);
 }
diff --git a/net/nfc/core.c b/net/nfc/core.c
index dc7a2404efd..09f54c599fe 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -1166,6 +1166,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
                rfkill_unregister(dev->rfkill);
                rfkill_destroy(dev->rfkill);
        }
+       dev->dev_up = false;
        device_unlock(&dev->dev);

        if (dev->ops->check_presence) {

The above solution has been tested, it is well synchronized.

Best regards,
Duoming Zhou

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Lin Ma]

Hello Jakub,

and hello there maintainers, when we tried to fix this race problem, we found another very weird issue as below

> 
> You can't use a single global variable, there can be many devices 
> each with their own lock.
> 
> Paolo suggested adding a lock, if spin lock doesn't fit the bill
> why not add a mutex?

The lock patch can be added to nfcmrvl code but we prefer to fix this in the NFC core layer hence every other driver that supports the firmware downloading task will be free of such a problem.

But when we analyze the race between the netlink tasks and the cleanup routine, we find some *condition checks* fail to fulfill their responsibility.

For example, we once though that the device_lock + device_is_registered check can help to fix the race as below.

  netlink task              |  cleanup routine
                            |
nfc_genl_fw_download        | nfc_unregister_device 
  nfc_fw_download           |   device_del 
    device_lock             |     device_lock
    // wait lock            |       kobject_del
    // ...                  |         ...
    device_is_registered    |     device_unlock    
      rc = -ENODEV          |

However, by dynamic debugging this issue, we find out that **even after the device_del**, the device_is_registered check still returns TRUE!

This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.

-> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")

To find out why, we find out the device_is_registered is implemented like below:

static inline int device_is_registered(struct device *dev)
{
	return dev->kobj.state_in_sysfs;
}

By debugging, we find out in normal case, this kobj.state_in_sysfs will be clear out like below

[#0] 0xffffffff81f0743a → __kobject_del(kobj=0xffff888009ca7018)
[#1] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
[#2] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
[#3] 0xffffffff827708db → device_del(dev=0xffff888009ca7018)
[#4] 0xffffffff8396496f → nfc_unregister_device(dev=0xffff888009ca7000)
[#5] 0xffffffff839850a9 → nci_unregister_device(ndev=0xffff888009ca3000)
[#6] 0xffffffff82811308 → nfcmrvl_nci_unregister_dev(priv=0xffff88800c805c00)
[#7] 0xffffffff83990c4f → nci_uart_tty_close(tty=0xffff88800b450000)
[#8] 0xffffffff820f6bd3 → tty_ldisc_kill(tty=0xffff88800b450000)
[#9] 0xffffffff820f7fb1 → tty_ldisc_hangup(tty=0xffff88800b450000, reinit=0x0)

The clear out is in function __kobject_del

static void __kobject_del(struct kobject *kobj)
{
   // ...

	kobj->state_in_sysfs = 0;
	kobj_kset_leave(kobj);
	kobj->parent = NULL;
}

The structure of device_del is like below

void device_del(struct device *dev)
{
	struct device *parent = dev->parent;
	struct kobject *glue_dir = NULL;
	struct class_interface *class_intf;
	unsigned int noio_flag;

	device_lock(dev);
	kill_device(dev);
	device_unlock(dev);
        
        // ...
        kobject_del(&dev->kobj);
	cleanup_glue_dir(dev, glue_dir);
	memalloc_noio_restore(noio_flag);
	put_device(parent);
}

In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.

This means the device_lock + device_is_registered is still prone to the data race. And this is not just the problem with firmware downloading. The all relevant netlink tasks that use the device_lock + device_is_registered is possible to be raced.

To this end, we will come out with two patches, one for fixing this device_is_registered by using another status variable instead. The other is the patch that reorders the code in nci_unregister_device.

Thanks
Lin Ma

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Greg KH]

On Thu, Apr 28, 2022 at 03:15:02PM +0800, Lin Ma wrote:
> Hello Jakub,
> 
> and hello there maintainers, when we tried to fix this race problem, we found another very weird issue as below
> 
> > 
> > You can't use a single global variable, there can be many devices 
> > each with their own lock.
> > 
> > Paolo suggested adding a lock, if spin lock doesn't fit the bill
> > why not add a mutex?
> 
> The lock patch can be added to nfcmrvl code but we prefer to fix this in the NFC core layer hence every other driver that supports the firmware downloading task will be free of such a problem.
> 
> But when we analyze the race between the netlink tasks and the cleanup routine, we find some *condition checks* fail to fulfill their responsibility.
> 
> For example, we once though that the device_lock + device_is_registered check can help to fix the race as below.
> 
>   netlink task              |  cleanup routine
>                             |
> nfc_genl_fw_download        | nfc_unregister_device 
>   nfc_fw_download           |   device_del 
>     device_lock             |     device_lock
>     // wait lock            |       kobject_del
>     // ...                  |         ...
>     device_is_registered    |     device_unlock    
>       rc = -ENODEV          |
> 
> However, by dynamic debugging this issue, we find out that **even after the device_del**, the device_is_registered check still returns TRUE!

You should not be making these types of checks outside of the driver
core.

> This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.

Please do not do that.

> 
> -> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")
> 
> To find out why, we find out the device_is_registered is implemented like below:
> 
> static inline int device_is_registered(struct device *dev)
> {
> 	return dev->kobj.state_in_sysfs;
> }
> 
> By debugging, we find out in normal case, this kobj.state_in_sysfs will be clear out like below
> 
> [#0] 0xffffffff81f0743a → __kobject_del(kobj=0xffff888009ca7018)
> [#1] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
> [#2] 0xffffffff81f07882 → kobject_del(kobj=0xffff888009ca7018)
> [#3] 0xffffffff827708db → device_del(dev=0xffff888009ca7018)
> [#4] 0xffffffff8396496f → nfc_unregister_device(dev=0xffff888009ca7000)
> [#5] 0xffffffff839850a9 → nci_unregister_device(ndev=0xffff888009ca3000)
> [#6] 0xffffffff82811308 → nfcmrvl_nci_unregister_dev(priv=0xffff88800c805c00)
> [#7] 0xffffffff83990c4f → nci_uart_tty_close(tty=0xffff88800b450000)
> [#8] 0xffffffff820f6bd3 → tty_ldisc_kill(tty=0xffff88800b450000)
> [#9] 0xffffffff820f7fb1 → tty_ldisc_hangup(tty=0xffff88800b450000, reinit=0x0)
> 
> The clear out is in function __kobject_del
> 
> static void __kobject_del(struct kobject *kobj)
> {
>    // ...
> 
> 	kobj->state_in_sysfs = 0;
> 	kobj_kset_leave(kobj);
> 	kobj->parent = NULL;
> }
> 
> The structure of device_del is like below
> 
> void device_del(struct device *dev)
> {
> 	struct device *parent = dev->parent;
> 	struct kobject *glue_dir = NULL;
> 	struct class_interface *class_intf;
> 	unsigned int noio_flag;
> 
> 	device_lock(dev);
> 	kill_device(dev);
> 	device_unlock(dev);
>         
>         // ...
>         kobject_del(&dev->kobj);
> 	cleanup_glue_dir(dev, glue_dir);
> 	memalloc_noio_restore(noio_flag);
> 	put_device(parent);
> }
> 
> In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.

Nor should it be.

> This means the device_lock + device_is_registered is still prone to the data race. And this is not just the problem with firmware downloading. The all relevant netlink tasks that use the device_lock + device_is_registered is possible to be raced.
> 
> To this end, we will come out with two patches, one for fixing this device_is_registered by using another status variable instead. The other is the patch that reorders the code in nci_unregister_device.

Why is this somehow unique to these devices?  Why do no other buses have
this issue?  Are you somehow allowing a code path that should not be
happening?

thanks,

greg k-h

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Lin Ma]

Hello Greg,


> 
> You should not be making these types of checks outside of the driver
> core.
> 
> > This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.
> 
> Please do not do that.
> 
> > 
> > -> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")
> > 
> <...>
> > 
> > In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.
> 
> Nor should it be.
> 

I may have mistakenly presented my point. In fact, there is nothing wrong with the device core, nothing to do with the internal of device_del and device_is_registered implementation. And, of course, we will not add any code or do any modification to the device/driver base code.

The point is the combination of device_is_registered + device_del, which is used in NFC core, is not safe.

That is to say, even the device_is_registered can return True even the device_del is executing in another thread.

(By debugging we think this is true, correct me if it is not)

Hence we want to add additional state in nfc_dev object to fix that, not going to add any state in device/driver core.

> > This means the device_lock + device_is_registered is still prone to the data race. And this is not just the problem with firmware downloading. The all relevant netlink tasks that use the device_lock + device_is_registered is possible to be raced.
> > 
> > To this end, we will come out with two patches, one for fixing this device_is_registered by using another status variable instead. The other is the patch that reorders the code in nci_unregister_device.
> 
> Why is this somehow unique to these devices?  Why do no other buses have
> this issue?  Are you somehow allowing a code path that should not be
> happening?
> 
> thanks,
> 
> greg k-h

In fact, by searching the device_is_registered() use cases, I found that most of them are used in drier code instead of in the network stack. I have no idea whether or not they suffer from similar problems and I will check that out.

Thanks
Lin

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Greg KH]

On Thu, Apr 28, 2022 at 03:55:01PM +0800, Lin Ma wrote:
> Hello Greg,
> 
> 
> > 
> > You should not be making these types of checks outside of the driver
> > core.
> > 
> > > This is by no means matching our expectations as one of our previous patch relies on the device_is_registered code.
> > 
> > Please do not do that.
> > 
> > > 
> > > -> the patch: 3e3b5dfcd16a ("NFC: reorder the logic in nfc_{un,}register_device")
> > > 
> > <...>
> > > 
> > > In another word, the device_del -> kobject_del -> __kobject_del is not protected by the device_lock.
> > 
> > Nor should it be.
> > 
> 
> I may have mistakenly presented my point. In fact, there is nothing wrong with the device core, nothing to do with the internal of device_del and device_is_registered implementation. And, of course, we will not add any code or do any modification to the device/driver base code.
> 
> The point is the combination of device_is_registered + device_del, which is used in NFC core, is not safe.

It shouldn't be, if you are using it properly :)

> That is to say, even the device_is_registered can return True even the device_del is executing in another thread.

Yes, you should almost never use that call.  Seems the nfc subsystem is
the most common user of it for some reason :(

> (By debugging we think this is true, correct me if it is not)
> 
> Hence we want to add additional state in nfc_dev object to fix that, not going to add any state in device/driver core.

What state are you trying to track here exactly?

thanks,

greg k-h

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Lin Ma]

Hello Greg,

> 
> It shouldn't be, if you are using it properly :)
> 
> [...]
> 
> Yes, you should almost never use that call.  Seems the nfc subsystem is
> the most common user of it for some reason :(

Cool, and I believe that the current nfc core code does not use it properly. :(

> 
> What state are you trying to track here exactly?
> 

Forget about the firmware downloading race that raised by Duoming in this channel,
all the netlink handler code in net/nfc/core.c depends on the device_is_registered
macro.

My idea is to introduce a patch like below:

 include/net/nfc/nfc.h |  1 +
 net/nfc/core.c        | 26 ++++++++++++++------------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h
index 5dee575fbe86..d84e53802b06 100644
--- a/include/net/nfc/nfc.h
+++ b/include/net/nfc/nfc.h
@@ -168,6 +168,7 @@ struct nfc_dev {
 	int targets_generation;
 	struct device dev;
 	bool dev_up;
+	bool dev_register;
 	bool fw_download_in_progress;
 	u8 rf_mode;
 	bool polling;
diff --git a/net/nfc/core.c b/net/nfc/core.c
index dc7a2404efdf..208e6bb0804e 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -38,7 +38,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
 
 	device_lock(&dev->dev);
 
-	if (!device_is_registered(&dev->dev)) {
+	if (!dev->dev_register) {
 		rc = -ENODEV;
 		goto error;
 	}
@@ -94,7 +94,7 @@ int nfc_dev_up(struct nfc_dev *dev)
 
 	device_lock(&dev->dev);
 
-	if (!device_is_registered(&dev->dev)) {
+	if (!dev->dev_register) {
 		rc = -ENODEV;
 		goto error;
 	}

[...]

@@ -1134,6 +1134,7 @@ int nfc_register_device(struct nfc_dev *dev)
 			dev->rfkill = NULL;
 		}
 	}
+	dev->dev_register = true;
 	device_unlock(&dev->dev);
 
 	rc = nfc_genl_device_added(dev);
@@ -1162,6 +1163,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
 			 "was removed\n", dev_name(&dev->dev));
 
 	device_lock(&dev->dev);
+	dev->dev_register = false;
 	if (dev->rfkill) {
 		rfkill_unregister(dev->rfkill);
 		rfkill_destroy(dev->rfkill);
-- 
2.35.1

The added dev_register variable can function like the original device_is_registered and does not race-able
because of the protection of device_lock.

I think after such a patch is adopted, the reorder version of patch from Duoming 
-> https://lists.openwall.net/netdev/2022/04/25/10
can be used to fix the firmware downloading bug.

Do you agree on this or should we use another macro that is suitable than device_is_registered?

> thanks,
> 
> greg k-h

Thanks
Lin

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Greg KH]

On Thu, Apr 28, 2022 at 04:49:18PM +0800, Lin Ma wrote:
> Hello Greg,
> 
> > 
> > It shouldn't be, if you are using it properly :)
> > 
> > [...]
> > 
> > Yes, you should almost never use that call.  Seems the nfc subsystem is
> > the most common user of it for some reason :(
> 
> Cool, and I believe that the current nfc core code does not use it properly. :(
> 
> > 
> > What state are you trying to track here exactly?
> > 
> 
> Forget about the firmware downloading race that raised by Duoming in this channel,
> all the netlink handler code in net/nfc/core.c depends on the device_is_registered
> macro.
> 
> My idea is to introduce a patch like below:
> 
>  include/net/nfc/nfc.h |  1 +
>  net/nfc/core.c        | 26 ++++++++++++++------------
>  2 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/nfc/nfc.h b/include/net/nfc/nfc.h
> index 5dee575fbe86..d84e53802b06 100644
> --- a/include/net/nfc/nfc.h
> +++ b/include/net/nfc/nfc.h
> @@ -168,6 +168,7 @@ struct nfc_dev {
>  	int targets_generation;
>  	struct device dev;
>  	bool dev_up;
> +	bool dev_register;
>  	bool fw_download_in_progress;
>  	u8 rf_mode;
>  	bool polling;
> diff --git a/net/nfc/core.c b/net/nfc/core.c
> index dc7a2404efdf..208e6bb0804e 100644
> --- a/net/nfc/core.c
> +++ b/net/nfc/core.c
> @@ -38,7 +38,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
>  
>  	device_lock(&dev->dev);
>  
> -	if (!device_is_registered(&dev->dev)) {
> +	if (!dev->dev_register) {
>  		rc = -ENODEV;
>  		goto error;
>  	}
> @@ -94,7 +94,7 @@ int nfc_dev_up(struct nfc_dev *dev)
>  
>  	device_lock(&dev->dev);
>  
> -	if (!device_is_registered(&dev->dev)) {
> +	if (!dev->dev_register) {
>  		rc = -ENODEV;
>  		goto error;
>  	}
> 
> [...]
> 
> @@ -1134,6 +1134,7 @@ int nfc_register_device(struct nfc_dev *dev)
>  			dev->rfkill = NULL;
>  		}
>  	}
> +	dev->dev_register = true;
>  	device_unlock(&dev->dev);
>  
>  	rc = nfc_genl_device_added(dev);
> @@ -1162,6 +1163,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
>  			 "was removed\n", dev_name(&dev->dev));
>  
>  	device_lock(&dev->dev);
> +	dev->dev_register = false;
>  	if (dev->rfkill) {
>  		rfkill_unregister(dev->rfkill);
>  		rfkill_destroy(dev->rfkill);
> -- 
> 2.35.1
> 
> The added dev_register variable can function like the original device_is_registered and does not race-able
> because of the protection of device_lock.

Yes, that looks better, but what is the root problem here that you are
trying to solve?  Why does NFC need this when no other subsystem does?

thansk,

greg k-h

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Jakub Kicinski]

On Thu, 28 Apr 2022 11:20:16 +0200 Greg KH wrote:
> > The added dev_register variable can function like the original
> > device_is_registered and does not race-able because of the
> > protection of device_lock.  
> 
> Yes, that looks better, but what is the root problem here that you are
> trying to solve?  Why does NFC need this when no other subsystem does?

Yeah :( The NFC and NCI locking is shaky at best, grounds-up redesign
with clear rules would be great... but then again I'm not sure if anyone
is actually using this code IRL, so the motivation to invest time is
rather weak.

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Lin Ma]

Hello there,

> 
> Yes, that looks better, 

Cool, thanks again for giving comments. :)

> but what is the root problem here that you are
> trying to solve?  Why does NFC need this when no other subsystem does?
> 

Well, in fact, me and Duoming are keep finding concurrency bugs that happen 
between the device cleanup/detach routine and other undergoing routines.

That is to say, when a device, no matter real or virtual, is detached from 
the machine, the kernel awake cleanup routine to reclaim the resource. 
In current case, the cleanup routine will call nfc_unregister_device().

Other routines, mainly from user-space system calls, need to be careful of 
the cleanup event. In another word, the kernel need to synchronize these 
routines to avoid race bugs.

In our practice, we find that many subsystems are prone to this type of bug.

For example, in bluetooth we fix

BT subsystem
* e2cb6b891ad2 ("bluetooth: eliminate the potential race condition when removing
the HCI controller")
* fa78d2d1d64f ("Bluetooth: fix data races in smp_unregister(), smp_del_chan()")
..

AX25 subsystem
* 1ade48d0c27d ("ax25: NPD bug when detaching AX25 device")
..

we currently focus on the net relevant subsystems and we now is auditing the NFC 
code.

In another word, all subsystems need to take care of the synchronization issues.
But seems that the solutions are varied between different subsystem. 

Empirically speaking, most of them use specific flags + specific locks to prevent
the race. 

In such cases, if the cleanup routine first hold the lock, the other routines will
wait on the locks. Since the cleanup routine write the specific flag, the other
routine, after check the specific flag, will be aware of the cleanup stuff and just
abort their tasks.
If the other routines first hold the lock, the cleanup routine just wait them to 
finish.

NFC here is special because it uses device_is_registered. I thought the author may
believe this macro is race free. However, it is not. So we need to replace this check
to make sure the netlink functions will 100 percent be aware of the cleanup routine
and abort the task if they grab the device_lock lately. Otherwise, the nelink routine
will call sub-layer code and possilby dereference resources that already freed.

For example, one of my recent fix 3e3b5dfcd16a ("NFC: reorder the logic in 
nfc_{un,}register_device") takes the suggestion from maintainer as he thought the 
device_is_registered is enough. And for now we find out this device_is_registered
is not enough.

If you are wondering if other subsystems use the combination of device_is_registered
as check to avoid the race, I have to say I don't know. If the answer is yes, that code
is possibly also prone to this type of concurrent bug.

> thansk,
> 
> greg k-h

Thanks again.

Lin Ma

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Greg KH]

On Thu, Apr 28, 2022 at 09:22:11PM +0800, Lin Ma wrote:
> Hello there,
> 
> > 
> > Yes, that looks better, 
> 
> Cool, thanks again for giving comments. :)
> 
> > but what is the root problem here that you are
> > trying to solve?  Why does NFC need this when no other subsystem does?
> > 
> 
> Well, in fact, me and Duoming are keep finding concurrency bugs that happen 
> between the device cleanup/detach routine and other undergoing routines.
> 
> That is to say, when a device, no matter real or virtual, is detached from 
> the machine, the kernel awake cleanup routine to reclaim the resource. 
> In current case, the cleanup routine will call nfc_unregister_device().
> 
> Other routines, mainly from user-space system calls, need to be careful of 
> the cleanup event. In another word, the kernel need to synchronize these 
> routines to avoid race bugs.
> 
> In our practice, we find that many subsystems are prone to this type of bug.
> 
> For example, in bluetooth we fix
> 
> BT subsystem
> * e2cb6b891ad2 ("bluetooth: eliminate the potential race condition when removing
> the HCI controller")
> * fa78d2d1d64f ("Bluetooth: fix data races in smp_unregister(), smp_del_chan()")
> ..
> 
> AX25 subsystem
> * 1ade48d0c27d ("ax25: NPD bug when detaching AX25 device")
> ..
> 
> we currently focus on the net relevant subsystems and we now is auditing the NFC 
> code.
> 
> In another word, all subsystems need to take care of the synchronization issues.
> But seems that the solutions are varied between different subsystem. 
> 
> Empirically speaking, most of them use specific flags + specific locks to prevent
> the race. 
> 
> In such cases, if the cleanup routine first hold the lock, the other routines will
> wait on the locks. Since the cleanup routine write the specific flag, the other
> routine, after check the specific flag, will be aware of the cleanup stuff and just
> abort their tasks.
> If the other routines first hold the lock, the cleanup routine just wait them to 
> finish.
> 
> NFC here is special because it uses device_is_registered. I thought the author may
> believe this macro is race free. However, it is not. So we need to replace this check
> to make sure the netlink functions will 100 percent be aware of the cleanup routine
> and abort the task if they grab the device_lock lately. Otherwise, the nelink routine
> will call sub-layer code and possilby dereference resources that already freed.
> 
> For example, one of my recent fix 3e3b5dfcd16a ("NFC: reorder the logic in 
> nfc_{un,}register_device") takes the suggestion from maintainer as he thought the 
> device_is_registered is enough. And for now we find out this device_is_registered
> is not enough.

How do you physically remove a NFC device from a system?  What types of
busses are they on?  If non-removable one, then odds are there's not
going to be many races so this is a low-priority thing.  If they can be
on removable busses (USB, PCI, etc.), then that's a bigger thing.

But again, the NFC bus code should handle all of this for the drivers,
there's nothing special about NFC that should warrant a special need for
this type of thing.

thanks,

greg k-h

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Lin Ma]

Hello there

> 
> How do you physically remove a NFC device from a system?  What types of
> busses are they on?  If non-removable one, then odds are there's not
> going to be many races so this is a low-priority thing.  If they can be
> on removable busses (USB, PCI, etc.), then that's a bigger thing.

Well, these are great questions we didn't even touch to yet.
For the BT, HAMRADIO, and NFC/NCI code, we simply use pseudo-terminal + line
discipline to emulate a malicious device from user-space (CAP_NET_ADMIN required).

We will then survey the actual physical bus for the IRL used NFC device.

> 
> But again, the NFC bus code should handle all of this for the drivers,
> there's nothing special about NFC that should warrant a special need for
> this type of thing.
> 

Agree, but in my opinion, every layer besides the bus code has to handle this race.

The cleanup routine is from
"down" to "up" like ... -> TTY -> NFCMRVL -> NCI -> NFC core
while the other routines, like netlink command is from "up" to "down"
user space -> netlink -> NFC -> NCI -> NFCMRVL -> ...

Their directions are totally reversed hence only each layer of the stack perform good
synchronization can the code be race free.

For example, this detaching event awake code in bus, the running task in higher layer
is not aware of this. The cleanup of each layer has to make sure any running code won't
cause use-after-free. 

> thanks,
> 
> greg k-h

Thanks
Lin Ma

Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Greg KH]

On Thu, Apr 28, 2022 at 09:53:28PM +0800, Lin Ma wrote:
> Hello there
> 
> > 
> > How do you physically remove a NFC device from a system?  What types of
> > busses are they on?  If non-removable one, then odds are there's not
> > going to be many races so this is a low-priority thing.  If they can be
> > on removable busses (USB, PCI, etc.), then that's a bigger thing.
> 
> Well, these are great questions we didn't even touch to yet.
> For the BT, HAMRADIO, and NFC/NCI code, we simply use pseudo-terminal + line
> discipline to emulate a malicious device from user-space (CAP_NET_ADMIN required).
> 
> We will then survey the actual physical bus for the IRL used NFC device.
> 
> > 
> > But again, the NFC bus code should handle all of this for the drivers,
> > there's nothing special about NFC that should warrant a special need for
> > this type of thing.
> > 
> 
> Agree, but in my opinion, every layer besides the bus code has to handle this race.
> 
> The cleanup routine is from
> "down" to "up" like ... -> TTY -> NFCMRVL -> NCI -> NFC core
> while the other routines, like netlink command is from "up" to "down"
> user space -> netlink -> NFC -> NCI -> NFCMRVL -> ...
> 
> Their directions are totally reversed hence only each layer of the stack perform good
> synchronization can the code be race free.
> 
> For example, this detaching event awake code in bus, the running task in higher layer
> is not aware of this. The cleanup of each layer has to make sure any running code won't
> cause use-after-free. 

That is what proper reference counting is supposed to be for.  Perhaps
you are running into a driver subsystem that is not doing that well, or
at all?

Try adding the needed references and the use-after-free should almost be
impossible to happen.

thanks,

greg k-h

Re: Re: [PATCH net v4] nfc: ... device_is_registered() is data race-able

[Lin Ma]

Hello there,

> 
> That is what proper reference counting is supposed to be for.  Perhaps
> you are running into a driver subsystem that is not doing that well, or
> at all?
> 
> Try adding the needed references and the use-after-free should almost be
> impossible to happen.
> 

That's true, if all the relevant resources are managed properly by the reference
count, everything will be easier.

> thanks,
> 
> greg k-h

Thanks
Lin Ma