* [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem
@ 2022-04-29 12:45 Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 1/2] nfc: replace improper check device_is_registered() in netlink related functions Duoming Zhou
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Duoming Zhou @ 2022-04-29 12:45 UTC (permalink / raw)
To: krzysztof.kozlowski, kuba, gregkh, linux-kernel
Cc: davem, edumazet, pabeni, netdev, alexander.deucher, broonie,
linma, Duoming Zhou
The first patch is used to replace improper checks in netlink related
functions of nfc core, the second patch is used to fix bugs in
nfcmrvl driver.
Duoming Zhou (2):
nfc: replace improper check device_is_registered() in netlink related
functions
nfc: nfcmrvl: main: reorder destructive operations in
nfcmrvl_nci_unregister_dev to avoid bugs
drivers/nfc/nfcmrvl/main.c | 2 +-
net/nfc/core.c | 29 ++++++++++++++---------------
2 files changed, 15 insertions(+), 16 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH net v6 1/2] nfc: replace improper check device_is_registered() in netlink related functions
2022-04-29 12:45 [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem Duoming Zhou
@ 2022-04-29 12:45 ` Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs Duoming Zhou
2022-05-01 12:30 ` [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Duoming Zhou @ 2022-04-29 12:45 UTC (permalink / raw)
To: krzysztof.kozlowski, kuba, gregkh, linux-kernel
Cc: davem, edumazet, pabeni, netdev, alexander.deucher, broonie,
linma, Duoming Zhou
The device_is_registered() in nfc core is used to check whether
nfc device is registered in netlink related functions such as
nfc_fw_download(), nfc_dev_up() and so on. Although device_is_registered()
is protected by device_lock, there is still a race condition between
device_del() and device_is_registered(). The root cause is that
kobject_del() in device_del() is not protected by device_lock.
(cleanup task) | (netlink task)
|
nfc_unregister_device | nfc_fw_download
device_del | device_lock
... | if (!device_is_registered)//(1)
kobject_del//(2) | ...
... | device_unlock
The device_is_registered() returns the value of state_in_sysfs and
the state_in_sysfs is set to zero in kobject_del(). If we pass check in
position (1), then set zero in position (2). As a result, the check
in position (1) is useless.
This patch uses bool variable instead of device_is_registered() to judge
whether the nfc device is registered, which is well synchronized.
Fixes: 3e256b8f8dfa ("NFC: add nfc subsystem core")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v6:
- Replace dev_register flag in v5 to shutting_down flag.
net/nfc/core.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/net/nfc/core.c b/net/nfc/core.c
index dc7a2404efd..5b286e1e0a6 100644
--- a/net/nfc/core.c
+++ b/net/nfc/core.c
@@ -38,7 +38,7 @@ int nfc_fw_download(struct nfc_dev *dev, const char *firmware_name)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -94,7 +94,7 @@ int nfc_dev_up(struct nfc_dev *dev)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -142,7 +142,7 @@ int nfc_dev_down(struct nfc_dev *dev)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -207,7 +207,7 @@ int nfc_start_poll(struct nfc_dev *dev, u32 im_protocols, u32 tm_protocols)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -246,7 +246,7 @@ int nfc_stop_poll(struct nfc_dev *dev)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -291,7 +291,7 @@ int nfc_dep_link_up(struct nfc_dev *dev, int target_index, u8 comm_mode)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -335,7 +335,7 @@ int nfc_dep_link_down(struct nfc_dev *dev)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -401,7 +401,7 @@ int nfc_activate_target(struct nfc_dev *dev, u32 target_idx, u32 protocol)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -448,7 +448,7 @@ int nfc_deactivate_target(struct nfc_dev *dev, u32 target_idx, u8 mode)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -495,7 +495,7 @@ int nfc_data_exchange(struct nfc_dev *dev, u32 target_idx, struct sk_buff *skb,
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
kfree_skb(skb);
goto error;
@@ -552,7 +552,7 @@ int nfc_enable_se(struct nfc_dev *dev, u32 se_idx)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -601,7 +601,7 @@ int nfc_disable_se(struct nfc_dev *dev, u32 se_idx)
device_lock(&dev->dev);
- if (!device_is_registered(&dev->dev)) {
+ if (dev->shutting_down) {
rc = -ENODEV;
goto error;
}
@@ -1134,6 +1134,7 @@ int nfc_register_device(struct nfc_dev *dev)
dev->rfkill = NULL;
}
}
+ dev->shutting_down = false;
device_unlock(&dev->dev);
rc = nfc_genl_device_added(dev);
@@ -1166,12 +1167,10 @@ void nfc_unregister_device(struct nfc_dev *dev)
rfkill_unregister(dev->rfkill);
rfkill_destroy(dev->rfkill);
}
+ dev->shutting_down = true;
device_unlock(&dev->dev);
if (dev->ops->check_presence) {
- device_lock(&dev->dev);
- dev->shutting_down = true;
- device_unlock(&dev->dev);
del_timer_sync(&dev->check_pres_timer);
cancel_work_sync(&dev->check_pres_work);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH net v6 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs
2022-04-29 12:45 [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 1/2] nfc: replace improper check device_is_registered() in netlink related functions Duoming Zhou
@ 2022-04-29 12:45 ` Duoming Zhou
2022-05-01 12:30 ` [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: Duoming Zhou @ 2022-04-29 12:45 UTC (permalink / raw)
To: krzysztof.kozlowski, kuba, gregkh, linux-kernel
Cc: davem, edumazet, pabeni, netdev, alexander.deucher, broonie,
linma, Duoming Zhou
There are destructive operations such as nfcmrvl_fw_dnld_abort and
gpio_free in nfcmrvl_nci_unregister_dev. The resources such as firmware,
gpio and so on could be destructed while the upper layer functions such as
nfcmrvl_fw_dnld_start and nfcmrvl_nci_recv_frame is executing, which leads
to double-free, use-after-free and null-ptr-deref bugs.
There are three situations that could lead to double-free bugs.
The first situation is shown below:
(Thread 1) | (Thread 2)
nfcmrvl_fw_dnld_start |
... | nfcmrvl_nci_unregister_dev
release_firmware() | nfcmrvl_fw_dnld_abort
kfree(fw) //(1) | fw_dnld_over
| release_firmware
... | kfree(fw) //(2)
| ...
The second situation is shown below:
(Thread 1) | (Thread 2)
nfcmrvl_fw_dnld_start |
... |
mod_timer |
(wait a time) |
fw_dnld_timeout | nfcmrvl_nci_unregister_dev
fw_dnld_over | nfcmrvl_fw_dnld_abort
release_firmware | fw_dnld_over
kfree(fw) //(1) | release_firmware
... | kfree(fw) //(2)
The third situation is shown below:
(Thread 1) | (Thread 2)
nfcmrvl_nci_recv_frame |
if(..->fw_download_in_progress)|
nfcmrvl_fw_dnld_recv_frame |
queue_work |
|
fw_dnld_rx_work | nfcmrvl_nci_unregister_dev
fw_dnld_over | nfcmrvl_fw_dnld_abort
release_firmware | fw_dnld_over
kfree(fw) //(1) | release_firmware
| kfree(fw) //(2)
The firmware struct is deallocated in position (1) and deallocated
in position (2) again.
The crash trace triggered by POC is like below:
BUG: KASAN: double-free or invalid-free in fw_dnld_over
Call Trace:
kfree
fw_dnld_over
nfcmrvl_nci_unregister_dev
nci_uart_tty_close
tty_ldisc_kill
tty_ldisc_hangup
__tty_hangup.part.0
tty_release
...
What's more, there are also use-after-free and null-ptr-deref bugs
in nfcmrvl_fw_dnld_start. If we deallocate firmware struct, gpio or
set null to the members of priv->fw_dnld in nfcmrvl_nci_unregister_dev,
then, we dereference firmware, gpio or the members of priv->fw_dnld in
nfcmrvl_fw_dnld_start, the UAF or NPD bugs will happen.
This patch reorders destructive operations after nci_unregister_device
in order to synchronize between cleanup routine and firmware download
routine.
The nci_unregister_device is well synchronized. If the device is
detaching, the firmware download routine will goto error. If firmware
download routine is executing, nci_unregister_device will wait until
firmware download routine is finished.
Fixes: 3194c6870158 ("NFC: nfcmrvl: add firmware download support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
Changes in v6:
- Make commit messages more clearer.
drivers/nfc/nfcmrvl/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 2fcf545012b..1a5284de434 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -183,6 +183,7 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
{
struct nci_dev *ndev = priv->ndev;
+ nci_unregister_device(ndev);
if (priv->ndev->nfc_dev->fw_download_in_progress)
nfcmrvl_fw_dnld_abort(priv);
@@ -191,7 +192,6 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
if (gpio_is_valid(priv->config.reset_n_io))
gpio_free(priv->config.reset_n_io);
- nci_unregister_device(ndev);
nci_free_device(ndev);
kfree(priv);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem
2022-04-29 12:45 [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 1/2] nfc: replace improper check device_is_registered() in netlink related functions Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs Duoming Zhou
@ 2022-05-01 12:30 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-05-01 12:30 UTC (permalink / raw)
To: Duoming Zhou
Cc: krzysztof.kozlowski, kuba, gregkh, linux-kernel, davem, edumazet,
pabeni, netdev, alexander.deucher, broonie, linma
Hello:
This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:
On Fri, 29 Apr 2022 20:45:49 +0800 you wrote:
> The first patch is used to replace improper checks in netlink related
> functions of nfc core, the second patch is used to fix bugs in
> nfcmrvl driver.
>
> Duoming Zhou (2):
> nfc: replace improper check device_is_registered() in netlink related
> functions
> nfc: nfcmrvl: main: reorder destructive operations in
> nfcmrvl_nci_unregister_dev to avoid bugs
>
> [...]
Here is the summary with links:
- [net,v6,1/2] nfc: replace improper check device_is_registered() in netlink related functions
https://git.kernel.org/netdev/net/c/da5c0f119203
- [net,v6,2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs
https://git.kernel.org/netdev/net/c/d270453a0d9e
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-01 12:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-29 12:45 [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 1/2] nfc: replace improper check device_is_registered() in netlink related functions Duoming Zhou
2022-04-29 12:45 ` [PATCH net v6 2/2] nfc: nfcmrvl: main: reorder destructive operations in nfcmrvl_nci_unregister_dev to avoid bugs Duoming Zhou
2022-05-01 12:30 ` [PATCH net v6 0/2] Replace improper checks and fix bugs in nfc subsystem patchwork-bot+netdevbpf
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).