* [linux-nfc] set dev->rfkill to NULL in device cleanup routine
@ 2021-09-01 7:39 LinMa
2021-09-01 8:27 ` [linux-nfc] " Krzysztof Kozlowski
0 siblings, 1 reply; 2+ messages in thread
From: LinMa @ 2021-09-01 7:39 UTC (permalink / raw)
To: linux-nfc
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().
The FREE chain is like
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).
Therefore, the below patch can be added.
Signed-off-by: Lin Ma <linma@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;
}
if (dev->ops->check_presence) {
--
2.32.0
_______________________________________________
Linux-nfc mailing list -- linux-nfc@lists.01.org
To unsubscribe send an email to linux-nfc-leave@lists.01.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
^ permalink raw reply related [flat|nested] 2+ messages in thread
* [linux-nfc] Re: set dev->rfkill to NULL in device cleanup routine
2021-09-01 7:39 [linux-nfc] set dev->rfkill to NULL in device cleanup routine LinMa
@ 2021-09-01 8:27 ` Krzysztof Kozlowski
0 siblings, 0 replies; 2+ messages in thread
From: Krzysztof Kozlowski @ 2021-09-01 8:27 UTC (permalink / raw)
To: LinMa, linux-nfc, David S. Miller, Jakub Kicinski, Networking,
Linux Kernel Mailing List
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/submitting-patches.rst#L124
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/submitting-patches.rst#L89
>
>
> Signed-off-by: Lin Ma <linma@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@lists.01.org
> To unsubscribe send an email to linux-nfc-leave@lists.01.org
> %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
>
Best regards,
Krzysztof
_______________________________________________
Linux-nfc mailing list -- linux-nfc@lists.01.org
To unsubscribe send an email to linux-nfc-leave@lists.01.org
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-09-01 8:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 7:39 [linux-nfc] set dev->rfkill to NULL in device cleanup routine LinMa
2021-09-01 8:27 ` [linux-nfc] " Krzysztof Kozlowski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).