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: be055b2f89
("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>
This commit is contained in:
parent
da3a87eeb9
commit
307d2e6ceb
@ -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;
|
||||
}
|
||||
|
||||
@ -1118,11 +1118,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) {
|
||||
@ -1131,6 +1127,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;
|
||||
}
|
||||
@ -1147,10 +1149,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);
|
||||
@ -1160,11 +1169,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);
|
||||
|
Loading…
Reference in New Issue
Block a user