On 01/09/2021 09:39, LinMa wrote:
In nfc_unregister_device() function, the dev->rfkill is forgotten
to set to NULL after the rfkill_destroy(). This may lead to possible cocurrency UAF in
other functions like nfc_dev_up().
Commit msg should be wrapper at 75 char.
https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submi...
Use also scripts/get_maintainers.pl to get list of people and lists
you need to CC. You skipped Networking maintainers and two mailing lists.
The FREE chain is like
Please trim multiple blank lines and organize the commit msg to be readable.
No need to paste existing code into the commit msg.
void nfc_unregister_device(struct nfc_dev *dev)
{
int rc;
pr_debug("dev_name=%s\n", dev_name(&dev->dev));
if (dev->rfkill) {
rfkill_unregister(dev->rfkill);
rfkill_destroy(dev->rfkill);
// ......
}
The USE chain is like
static int nfc_genl_dev_up(struct sk_buff *skb, struct genl_info *info)
{
struct nfc_dev *dev;
int rc;
u32 idx;
if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
return -EINVAL;
idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
dev = nfc_get_device(idx);
if (!dev)
return -ENODEV;
rc = nfc_dev_up(dev);
// ......
}
int nfc_dev_up(struct nfc_dev *dev)
{
int rc = 0;
pr_debug("dev_name=%s\n", dev_name(&dev->dev));
device_lock(&dev->dev);
if (dev->rfkill && rfkill_blocked(dev->rfkill)) { // dev->rfkill is
not NULL here
rc = -ERFKILL;
goto error;
}
// ......
}
The FREE chain and USE chain can be like below (as there is no locking protection).
Something is missing.
Therefore, the below patch can be added.
Use imperative form:
https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submi...
Signed-off-by: Lin Ma <linma(a)zju.edu.cn>
---
net/nfc/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/nfc/core.c b/net/nfc/core.c
index 573c80c6ff7a..d0b3224e65d7 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -1157,6 +1157,7 @@ void nfc_unregister_device(struct nfc_dev *dev)
if (dev->rfkill) {
rfkill_unregister(dev->rfkill);
rfkill_destroy(dev->rfkill);
+ dev->rfkill = NULL;
This is not a valid patch. Does not match the code.
For example, use git format-patch and git send-email.
About the topic:
Your code does not prevent a race condition, since you say there is no
locking. Even if you move dev->rfkill=NULL before rfkill_unregister(),
still nfc_dev_up() could happen between.
The questions are:
1. Whether nfc_unregister_device() can happen after nfc_get_device()?
2. Whether netlink nfc_genl_dev_up() can happen after nfc_unregister_device()
started.
}
if (dev->ops->check_presence) {
--
2.32.0
_______________________________________________
Linux-nfc mailing list -- linux-nfc(a)lists.01.org
To unsubscribe send an email to linux-nfc-leave(a)lists.01.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
Best regards,
Krzysztof