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

[ Upstream commit 3e3b5dfcd16a3e254aab61bd1e8c417dd4503102 ]

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>
Signed-off-by: Sasha Levin <sashal@kernel.org>

1 files changed, 18 insertions, 14 deletions

diff --git a/net/nfc/core.c b/net/nfc/core.c
index 1471e4b0aa2c..8c7f221e1d12 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -106,13 +106,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;
         }
 
@@ -1120,11 +1120,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) {
@@ -1133,6 +1129,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;
 }
@@ -1149,10 +1151,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);
@@ -1162,11 +1171,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);