Commit 3e3b5dfc authored by Lin Ma's avatar Lin Ma Committed by Jakub Kicinski

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: default avatarLin Ma <linma@zju.edu.cn>
Fixes: be055b2f ("NFC: RFKILL support")
Reviewed-by: default avatarJakub Kicinski <kuba@kernel.org>
Reviewed-by: default avatarKrzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Link: https://lore.kernel.org/r/20211116152652.19217-1-linma@zju.edu.cnSigned-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parent 86cdf8e3
...@@ -94,13 +94,13 @@ int nfc_dev_up(struct nfc_dev *dev) ...@@ -94,13 +94,13 @@ int nfc_dev_up(struct nfc_dev *dev)
device_lock(&dev->dev); device_lock(&dev->dev);
if (dev->rfkill && rfkill_blocked(dev->rfkill)) { if (!device_is_registered(&dev->dev)) {
rc = -ERFKILL; rc = -ENODEV;
goto error; goto error;
} }
if (!device_is_registered(&dev->dev)) { if (dev->rfkill && rfkill_blocked(dev->rfkill)) {
rc = -ENODEV; rc = -ERFKILL;
goto error; goto error;
} }
...@@ -1125,11 +1125,7 @@ int nfc_register_device(struct nfc_dev *dev) ...@@ -1125,11 +1125,7 @@ int nfc_register_device(struct nfc_dev *dev)
if (rc) if (rc)
pr_err("Could not register llcp device\n"); pr_err("Could not register llcp device\n");
rc = nfc_genl_device_added(dev); device_lock(&dev->dev);
if (rc)
pr_debug("The userspace won't be notified that the device %s was added\n",
dev_name(&dev->dev));
dev->rfkill = rfkill_alloc(dev_name(&dev->dev), &dev->dev, dev->rfkill = rfkill_alloc(dev_name(&dev->dev), &dev->dev,
RFKILL_TYPE_NFC, &nfc_rfkill_ops, dev); RFKILL_TYPE_NFC, &nfc_rfkill_ops, dev);
if (dev->rfkill) { if (dev->rfkill) {
...@@ -1138,6 +1134,12 @@ int nfc_register_device(struct nfc_dev *dev) ...@@ -1138,6 +1134,12 @@ int nfc_register_device(struct nfc_dev *dev)
dev->rfkill = NULL; 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; return 0;
} }
...@@ -1154,10 +1156,17 @@ void nfc_unregister_device(struct nfc_dev *dev) ...@@ -1154,10 +1156,17 @@ void nfc_unregister_device(struct nfc_dev *dev)
pr_debug("dev_name=%s\n", dev_name(&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) { if (dev->rfkill) {
rfkill_unregister(dev->rfkill); rfkill_unregister(dev->rfkill);
rfkill_destroy(dev->rfkill); rfkill_destroy(dev->rfkill);
} }
device_unlock(&dev->dev);
if (dev->ops->check_presence) { if (dev->ops->check_presence) {
device_lock(&dev->dev); device_lock(&dev->dev);
...@@ -1167,11 +1176,6 @@ void nfc_unregister_device(struct nfc_dev *dev) ...@@ -1167,11 +1176,6 @@ void nfc_unregister_device(struct nfc_dev *dev)
cancel_work_sync(&dev->check_pres_work); 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); nfc_llcp_unregister_device(dev);
mutex_lock(&nfc_devlist_mutex); mutex_lock(&nfc_devlist_mutex);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment