NFC: reorder the logic in nfc_{un,}register_device

There is a potential UAF between the unregistration routine and the NFC
netlink operations.

The race that cause that UAF can be shown as below:

 (FREE)                      |  (USE)
nfcmrvl_nci_unregister_dev   |  nfc_genl_dev_up
  nci_close_device           |
  nci_unregister_device      |    nfc_get_device
    nfc_unregister_device    |    nfc_dev_up
      rfkill_destory         |
      device_del             |      rfkill_blocked
  ...                        |    ...

The root cause for this race is concluded below:
1. The rfkill_blocked (USE) in nfc_dev_up is supposed to be placed after
the device_is_registered check.
2. Since the netlink operations are possible just after the device_add
in nfc_register_device, the nfc_dev_up() can happen anywhere during the
rfkill creation process, which leads to data race.

This patch reorder these actions to permit
1. Once device_del is finished, the nfc_dev_up cannot dereference the
rfkill object.
2. The rfkill_register need to be placed after the device_add of nfc_dev
because the parent device need to be created first. So this patch keeps
the order but inject device_lock to prevent the data race.

Signed-off-by: Lin Ma <linma@zju.edu.cn>
Fixes: be055b2f89b5 ("NFC: RFKILL support")
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Link: https://lore.kernel.org/r/20211116152652.19217-1-linma@zju.edu.cn
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/net/nfc/core.c b/net/nfc/core.c
index 3c645c1d99c9b63f3519d53e717f2c878c4efcb6..dc7a2404efdf9c7c17e1cf25a39df0b5d3afc76c 100644 (file)
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -94,13 +94,13 @@ int nfc_dev_up(struct nfc_dev *dev)
 
        device_lock(&dev->dev);
 
-       if (dev->rfkill && rfkill_blocked(dev->rfkill)) {
-               rc = -ERFKILL;
+       if (!device_is_registered(&dev->dev)) {
+               rc = -ENODEV;
                goto error;
        }
 
-       if (!device_is_registered(&dev->dev)) {
-               rc = -ENODEV;
+       if (dev->rfkill && rfkill_blocked(dev->rfkill)) {
+               rc = -ERFKILL;
                goto error;
        }
 
@@ -1125,11 +1125,7 @@ int nfc_register_device(struct nfc_dev *dev)
        if (rc)
                pr_err("Could not register llcp device\n");
 
-       rc = nfc_genl_device_added(dev);
-       if (rc)
-               pr_debug("The userspace won't be notified that the device %s was added\n",
-                        dev_name(&dev->dev));
-
+       device_lock(&dev->dev);
        dev->rfkill = rfkill_alloc(dev_name(&dev->dev), &dev->dev,
                                   RFKILL_TYPE_NFC, &nfc_rfkill_ops, dev);
        if (dev->rfkill) {
@@ -1138,6 +1134,12 @@ int nfc_register_device(struct nfc_dev *dev)
                        dev->rfkill = NULL;
                }
        }
+       device_unlock(&dev->dev);
+
+       rc = nfc_genl_device_added(dev);
+       if (rc)
+               pr_debug("The userspace won't be notified that the device %s was added\n",
+                        dev_name(&dev->dev));
 
        return 0;
 }
@@ -1154,10 +1156,17 @@ void nfc_unregister_device(struct nfc_dev *dev)
 
        pr_debug("dev_name=%s\n", dev_name(&dev->dev));
 
+       rc = nfc_genl_device_removed(dev);
+       if (rc)
+               pr_debug("The userspace won't be notified that the device %s "
+                        "was removed\n", dev_name(&dev->dev));
+
+       device_lock(&dev->dev);
        if (dev->rfkill) {
                rfkill_unregister(dev->rfkill);
                rfkill_destroy(dev->rfkill);
        }
+       device_unlock(&dev->dev);
 
        if (dev->ops->check_presence) {
                device_lock(&dev->dev);
@@ -1167,11 +1176,6 @@ void nfc_unregister_device(struct nfc_dev *dev)
                cancel_work_sync(&dev->check_pres_work);
        }
 
-       rc = nfc_genl_device_removed(dev);
-       if (rc)
-               pr_debug("The userspace won't be notified that the device %s "
-                        "was removed\n", dev_name(&dev->dev));
-
        nfc_llcp_unregister_device(dev);
 
        mutex_lock(&nfc_devlist_mutex);