* [PATCH] drivers: nfc: nfcmrvl: fix UAF bug in nfcmrvl_nci_unregister_dev()
@ 2022-04-09 13:58 Duoming Zhou
2022-04-09 15:10 ` Krzysztof Kozlowski
0 siblings, 1 reply; 4+ messages in thread
From: Duoming Zhou @ 2022-04-09 13:58 UTC (permalink / raw)
To: linux-kernel; +Cc: netdev, alexander.deucher, gregkh, davem, krzk, Duoming Zhou
There is a potential UAF bug in nfcmrvl usb driver between
unregister and resume operation.
The race that cause that UAF can be shown as below:
(FREE) | (USE)
| nfcmrvl_resume
| nfcmrvl_submit_bulk_urb
| nfcmrvl_bulk_complete
| nfcmrvl_nci_recv_frame
| nfcmrvl_fw_dnld_recv_frame
| skb_queue_tail
nfcmrvl_disconnect |
nfcmrvl_nci_unregister_dev |
nfcmrvl_fw_dnld_deinit | ...
destroy_workqueue //(1) |
... | queue_work //(2)
When nfcmrvl usb driver is resuming, we detach the device.
The workqueue is destroyed in position (1), but it will be
latter used in position (2), which leads to data race.
This patch reorders the nfcmrvl_fw_dnld_deinit after
nci_unregister_device in order to prevent UAF. Because
nci_unregister_device will not return until finish all
operations from upper layer.
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
drivers/nfc/nfcmrvl/main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
index 2fcf545012b..5ed17b23ee8 100644
--- a/drivers/nfc/nfcmrvl/main.c
+++ b/drivers/nfc/nfcmrvl/main.c
@@ -186,12 +186,11 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
if (priv->ndev->nfc_dev->fw_download_in_progress)
nfcmrvl_fw_dnld_abort(priv);
- nfcmrvl_fw_dnld_deinit(priv);
-
if (gpio_is_valid(priv->config.reset_n_io))
gpio_free(priv->config.reset_n_io);
nci_unregister_device(ndev);
+ nfcmrvl_fw_dnld_deinit(priv);
nci_free_device(ndev);
kfree(priv);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers: nfc: nfcmrvl: fix UAF bug in nfcmrvl_nci_unregister_dev()
2022-04-09 13:58 [PATCH] drivers: nfc: nfcmrvl: fix UAF bug in nfcmrvl_nci_unregister_dev() Duoming Zhou
@ 2022-04-09 15:10 ` Krzysztof Kozlowski
2022-04-10 8:30 ` duoming
0 siblings, 1 reply; 4+ messages in thread
From: Krzysztof Kozlowski @ 2022-04-09 15:10 UTC (permalink / raw)
To: Duoming Zhou, linux-kernel; +Cc: netdev, alexander.deucher, gregkh, davem
On 09/04/2022 15:58, Duoming Zhou wrote:
> There is a potential UAF bug in nfcmrvl usb driver between
> unregister and resume operation.
>
> The race that cause that UAF can be shown as below:
>
> (FREE) | (USE)
> | nfcmrvl_resume
> | nfcmrvl_submit_bulk_urb
> | nfcmrvl_bulk_complete
> | nfcmrvl_nci_recv_frame
> | nfcmrvl_fw_dnld_recv_frame
> | skb_queue_tail
> nfcmrvl_disconnect |
> nfcmrvl_nci_unregister_dev |
> nfcmrvl_fw_dnld_deinit | ...
> destroy_workqueue //(1) |
> ... | queue_work //(2)
>
> When nfcmrvl usb driver is resuming, we detach the device.
> The workqueue is destroyed in position (1), but it will be
> latter used in position (2), which leads to data race.
I miss here something. How can you queue work on a destroyed workqueue?
When workqueue is destroyed, no more work should be executed. Unless you
mean the case that draining the work (during destroying, not after)
causes the (2) to happen?
> This patch reorders the nfcmrvl_fw_dnld_deinit after
> nci_unregister_device in order to prevent UAF. Because
> nci_unregister_device will not return until finish all
> operations from upper layer.
>
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
> drivers/nfc/nfcmrvl/main.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> index 2fcf545012b..5ed17b23ee8 100644
> --- a/drivers/nfc/nfcmrvl/main.c
> +++ b/drivers/nfc/nfcmrvl/main.c
> @@ -186,12 +186,11 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
> if (priv->ndev->nfc_dev->fw_download_in_progress)
> nfcmrvl_fw_dnld_abort(priv);
>
> - nfcmrvl_fw_dnld_deinit(priv);
> -
> if (gpio_is_valid(priv->config.reset_n_io))
> gpio_free(priv->config.reset_n_io);
>
> nci_unregister_device(ndev);
> + nfcmrvl_fw_dnld_deinit(priv);
The new order matches reverse-probe, so actually makes sense.
> nci_free_device(ndev);
> kfree(priv);
> }
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: Re: [PATCH] drivers: nfc: nfcmrvl: fix UAF bug in nfcmrvl_nci_unregister_dev()
2022-04-09 15:10 ` Krzysztof Kozlowski
@ 2022-04-10 8:30 ` duoming
2022-04-10 9:17 ` Krzysztof Kozlowski
0 siblings, 1 reply; 4+ messages in thread
From: duoming @ 2022-04-10 8:30 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-kernel, netdev, alexander.deucher, gregkh, davem
Hello,
I am sorry for the delay.
On Sat, 9 Apr 2022 17:10:05 +0200 Krzysztof Kozlowski wrote:
> > There is a potential UAF bug in nfcmrvl usb driver between
> > unregister and resume operation.
> >
> > The race that cause that UAF can be shown as below:
> >
> > (FREE) | (USE)
> > | nfcmrvl_resume
> > | nfcmrvl_submit_bulk_urb
> > | nfcmrvl_bulk_complete
> > | nfcmrvl_nci_recv_frame
> > | nfcmrvl_fw_dnld_recv_frame
> > | skb_queue_tail
> > nfcmrvl_disconnect |
> > nfcmrvl_nci_unregister_dev |
> > nfcmrvl_fw_dnld_deinit | ...
> > destroy_workqueue //(1) |
> > ... | queue_work //(2)
> >
> > When nfcmrvl usb driver is resuming, we detach the device.
> > The workqueue is destroyed in position (1), but it will be
> > latter used in position (2), which leads to data race.
>
> I miss here something. How can you queue work on a destroyed workqueue?
> When workqueue is destroyed, no more work should be executed. Unless you
> mean the case that draining the work (during destroying, not after)
> causes the (2) to happen?
Sorry, I make a mistake in my patch. The destroy_workqueue() will not
return until all work is finished. So the UAF bug is not exist.
Actually, there is a double free bug in nfcmrvl_nci_unregister_dev(). The root
cause is shown below:
(FREE) | (USE)
| nfcmrvl_resume
| nfcmrvl_submit_bulk_urb
| nfcmrvl_bulk_complete
| nfcmrvl_nci_recv_frame
| nfcmrvl_fw_dnld_recv_frame
| queue_work
| fw_dnld_rx_work
| fw_dnld_over
| release_firmware
| kfree(fw); //(1)
nfcmrvl_disconnect |
nfcmrvl_nci_unregister_dev |
nfcmrvl_fw_dnld_abort |
fw_dnld_over | ...
if (priv->fw_dnld.fw) |
release_firmware |
kfree(fw); //(2) |
... | fw = NULL;
When nfcmrvl usb driver is resuming, we detach the device.
The release_firmware() will deallocate firmware in position (1),
but firmware will be deallocate again in position (2), which
leads to double free.
> > This patch reorders the nfcmrvl_fw_dnld_deinit after
> > nci_unregister_device in order to prevent UAF. Because
> > nci_unregister_device will not return until finish all
> > operations from upper layer.
> >
> > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> > ---
> > drivers/nfc/nfcmrvl/main.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
> > index 2fcf545012b..5ed17b23ee8 100644
> > --- a/drivers/nfc/nfcmrvl/main.c
> > +++ b/drivers/nfc/nfcmrvl/main.c
> > @@ -186,12 +186,11 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
> > if (priv->ndev->nfc_dev->fw_download_in_progress)
> > nfcmrvl_fw_dnld_abort(priv);
> >
> > - nfcmrvl_fw_dnld_deinit(priv);
> > -
> > if (gpio_is_valid(priv->config.reset_n_io))
> > gpio_free(priv->config.reset_n_io);
> >
> > nci_unregister_device(ndev);
> > + nfcmrvl_fw_dnld_deinit(priv);
>
> The new order matches reverse-probe, so actually makes sense.
>
> > nci_free_device(ndev);
> > kfree(priv);
> > }
I will send "[PATCH] drivers: nfc: nfcmrvl: fix double free bug in
nfcmrvl_nci_unregister_dev()" as soon as possible.
Best regards,
Duoming Zhou
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drivers: nfc: nfcmrvl: fix UAF bug in nfcmrvl_nci_unregister_dev()
2022-04-10 8:30 ` duoming
@ 2022-04-10 9:17 ` Krzysztof Kozlowski
0 siblings, 0 replies; 4+ messages in thread
From: Krzysztof Kozlowski @ 2022-04-10 9:17 UTC (permalink / raw)
To: duoming; +Cc: linux-kernel, netdev, alexander.deucher, gregkh, davem
On 10/04/2022 10:30, duoming@zju.edu.cn wrote:
(...)
>
> (FREE) | (USE)
> | nfcmrvl_resume
> | nfcmrvl_submit_bulk_urb
> | nfcmrvl_bulk_complete
> | nfcmrvl_nci_recv_frame
> | nfcmrvl_fw_dnld_recv_frame
> | queue_work
> | fw_dnld_rx_work
> | fw_dnld_over
> | release_firmware
> | kfree(fw); //(1)
> nfcmrvl_disconnect |
> nfcmrvl_nci_unregister_dev |
> nfcmrvl_fw_dnld_abort |
> fw_dnld_over | ...
> if (priv->fw_dnld.fw) |
> release_firmware |
> kfree(fw); //(2) |
> ... | fw = NULL;
>
> When nfcmrvl usb driver is resuming, we detach the device.
> The release_firmware() will deallocate firmware in position (1),
> but firmware will be deallocate again in position (2), which
> leads to double free.
Indeed. The code looks racy. It uses the fw_download_in_progress
variable which in core is partially protected with device_lock(). Moving
code around might not solve the issue entirely because there is no
synchronization here.
>
>>> This patch reorders the nfcmrvl_fw_dnld_deinit after
>>> nci_unregister_device in order to prevent UAF. Because
>>> nci_unregister_device will not return until finish all
>>> operations from upper layer.
>>>
>>> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
>>> ---
>>> drivers/nfc/nfcmrvl/main.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/nfc/nfcmrvl/main.c b/drivers/nfc/nfcmrvl/main.c
>>> index 2fcf545012b..5ed17b23ee8 100644
>>> --- a/drivers/nfc/nfcmrvl/main.c
>>> +++ b/drivers/nfc/nfcmrvl/main.c
>>> @@ -186,12 +186,11 @@ void nfcmrvl_nci_unregister_dev(struct nfcmrvl_private *priv)
>>> if (priv->ndev->nfc_dev->fw_download_in_progress)
>>> nfcmrvl_fw_dnld_abort(priv);
>>>
>>> - nfcmrvl_fw_dnld_deinit(priv);
>>> -
>>> if (gpio_is_valid(priv->config.reset_n_io))
>>> gpio_free(priv->config.reset_n_io);
>>>
>>> nci_unregister_device(ndev);
>>> + nfcmrvl_fw_dnld_deinit(priv);
>>
>> The new order matches reverse-probe, so actually makes sense.
>>
>>> nci_free_device(ndev);
>>> kfree(priv);
>>> }
>
> I will send "[PATCH] drivers: nfc: nfcmrvl: fix double free bug in
> nfcmrvl_nci_unregister_dev()" as soon as possible.
Thanks!
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-10 9:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-09 13:58 [PATCH] drivers: nfc: nfcmrvl: fix UAF bug in nfcmrvl_nci_unregister_dev() Duoming Zhou
2022-04-09 15:10 ` Krzysztof Kozlowski
2022-04-10 8:30 ` duoming
2022-04-10 9:17 ` 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).