[PATCH 0/3] Fix double free bugs and UAF bug in nfcmrvl module

[PATCH 0/3] Fix double free bugs and UAF bug in nfcmrvl module

[Duoming Zhou]

We add lock and check in fw_dnld_over() and nfcmrvl_fw_dnld_start(),
in order to synchronize among different threads that operate on
firmware.

Duoming Zhou (3):
  drivers: nfc: nfcmrvl: fix double free bugs caused by fw_dnld_over()
  drivers: nfc: nfcmrvl: fix double free bug in nfc_fw_download_done()
  drivers: nfc: nfcmrvl: fix use-after-free bug in
    nfcmrvl_fw_dnld_start()

 drivers/nfc/nfcmrvl/fw_dnld.c | 14 +++++++++++---
 drivers/nfc/nfcmrvl/fw_dnld.h |  2 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH V3 1/3] drivers: nfc: nfcmrvl: fix double free bugs caused by fw_dnld_over()

[Duoming Zhou]

There are potential double free bugs in nfcmrvl usb driver among
fw_dnld_rx_work(), fw_dnld_timeout() and nfcmrvl_nci_unregister_dev().
All these three functions will call fw_dnld_over(). The fw_dnld_rx_work()
is a work item, the fw_dnld_timeout() is a timer handler and the
nfcmrvl_nci_unregister_dev() is called when nfcmrvl_nci device is
detaching. So these three functions could execute concurrently.

The race between fw_dnld_rx_work() and nfcmrvl_nci_unregister_dev()
can be shown as below:

   (Thread 1)               |      (Thread 2)
                            |       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)       |
     ...                    |         priv->fw_dnld.fw = NULL;

When the fw_dnld_rx_work work item is executing, we detach the device.
The release_firmware() will deallocate firmware in position (1),
but firmware will be deallocated again in position (2), which
leads to double free.

The race between fw_dnld_rx_work() and fw_dnld_timeout()
can be shown as below:

   (Thread 1)               |      (Thread 2)
                            |       fw_dnld_rx_work
                            |        fw_dnld_over
                            |         release_firmware
                            |          kfree(fw); //(1)
fw_dnld_timeout             |
 fw_dnld_over               |         ...
  if (priv->fw_dnld.fw)     |
   release_firmware         |
    kfree(fw); //(2)        |
     ...                    |         priv->fw_dnld.fw = NULL;

The release_firmware() will deallocate firmware in position (1),
but firmware will be deallocated again in position (2), which
leads to double free.

The race between fw_dnld_timeout() and nfcmrvl_nci_unregister_dev()
can be shown as below.

   (Thread 1)               |      (Thread 2)
                            |     nfcmrvl_disconnect
                            |      nfcmrvl_nci_unregister_dev
                            |       nfcmrvl_fw_dnld_abort
                            |        fw_dnld_over
                            |         release_firmware
                            |          kfree(fw); //(1)
fw_dnld_timeout             |
 fw_dnld_over               |         ...
  if (priv->fw_dnld.fw)     |
   release_firmware         |
    kfree(fw); //(2)        |
     ...                    |         priv->fw_dnld.fw = NULL;

The release_firmware() will deallocate firmware in position (1),
but firmware will be deallocated again in position (2), which
leads to double free.

This patch adds spin_lock_irq in fw_dnld_over() in order to synchronize
among different threads that call fw_dnld_over(). The priv->fw_dnld.fw will
be set to NULL after work item is finished and fw_dnld_over() called by
other threads will check whether priv->fw_dnld.fw is NULL. So the double
free bug could be prevented.

Fixes: 3194c6870158e3 ("NFC: nfcmrvl: add firmware download support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
Reviewed-by: Lin Ma <linma@zju.edu.cn>
---
Changes in V3:
  - Make commit message more clearer.
  - Use spin_lock_irq to synchronize.

 drivers/nfc/nfcmrvl/fw_dnld.c | 3 +++
 drivers/nfc/nfcmrvl/fw_dnld.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index e83f65596a8..c22a4556db5 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -92,12 +92,14 @@ static struct sk_buff *alloc_lc_skb(struct nfcmrvl_private *priv, uint8_t plen)
 
 static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
 {
+	spin_lock_irq(&priv->fw_dnld.lock);
 	if (priv->fw_dnld.fw) {
 		release_firmware(priv->fw_dnld.fw);
 		priv->fw_dnld.fw = NULL;
 		priv->fw_dnld.header = NULL;
 		priv->fw_dnld.binary_config = NULL;
 	}
+	spin_unlock_irq(&priv->fw_dnld.lock);
 
 	atomic_set(&priv->ndev->cmd_cnt, 0);
 
@@ -451,6 +453,7 @@ int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv)
 	if (!priv->fw_dnld.rx_wq)
 		return -ENOMEM;
 	skb_queue_head_init(&priv->fw_dnld.rx_q);
+	spin_lock_init(&priv->fw_dnld.lock);
 	return 0;
 }
 
diff --git a/drivers/nfc/nfcmrvl/fw_dnld.h b/drivers/nfc/nfcmrvl/fw_dnld.h
index 7c4d91b0191..6974c307947 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.h
+++ b/drivers/nfc/nfcmrvl/fw_dnld.h
@@ -75,6 +75,8 @@ struct nfcmrvl_fw_dnld {
 	struct sk_buff_head rx_q;
 
 	struct timer_list timer;
+	/* To synchronize among different threads that call firmware.*/
+	spinlock_t lock;
 };
 
 int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv);
-- 
2.17.1

[PATCH V2 2/3] drivers: nfc: nfcmrvl: fix double free bug in nfc_fw_download_done()

[Duoming Zhou]

There are potential double free bug in nfc_fw_download_done().
The timer handler fw_dnld_timeout() and work item fw_dnld_rx_work()
could both reach in fw_dnld_over() and nfc_fw_download_done() is not
protected by any lock, which leads to double free.

The race between fw_dnld_rx_work() and fw_dnld_timeout()
can be shown as below:

   (Thread 1)               |      (Thread 2)
fw_dnld_timeout             |  fw_dnld_rx_work
                            |   fw_dnld_over
 fw_dnld_over               |    nfc_fw_download_done
  nfc_fw_download_done      |     nfc_genl_fw_download_done
   nfc_genl_fw_download_done|      nlmsg_free(msg)  //(1)
    nlmsg_free(msg) //(2)   |      ...
     ...                    |

The nlmsg_free() will deallocate sk_buff in position (1), but
nlmsg_free will be deallocated again in position (2), which
leads to double free.

This patch adds spin_lock_irq() and check in fw_dnld_over()
in order to synchronize among different threads that call
fw_dnld_over(). So the double free bug could be prevented.

Fixes: 3194c6870158e3 ("NFC: nfcmrvl: add firmware download support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
Reviewed-by: Lin Ma <linma@zju.edu.cn>
---
Changes in V2:
  - Fix the check in spin_lock_irq.

 drivers/nfc/nfcmrvl/fw_dnld.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index c22a4556db5..bb9e7e2bdec 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -115,8 +115,10 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
 		/* failed, halt the chip to avoid power consumption */
 		nfcmrvl_chip_halt(priv);
 	}
-
-	nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
+	spin_lock_irq(&priv->fw_dnld.lock);
+	if (priv->ndev->nfc_dev->fw_download_in_progress)
+		nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
+	spin_unlock_irq(&priv->fw_dnld.lock);
 }
 
 static void fw_dnld_timeout(struct timer_list *t)
-- 
2.17.1

[PATCH 3/3] drivers: nfc: nfcmrvl: fix use-after-free bug in nfcmrvl_fw_dnld_start()

[Duoming Zhou]

There are potential use-after-free bug in nfcmrvl_fw_dnld_start().
The race between nfcmrvl_disconnect() and nfcmrvl_fw_dnld_start()
can be shown as below:

   (USE)                      |      (FREE)
                              | nfcmrvl_disconnect
                              |  nfcmrvl_nci_unregister_dev
                              |   nfcmrvl_fw_dnld_abort
                              |    fw_dnld_over
nfcmrvl_fw_dnld_start         |     release_firmware
 ...                          |      kfree(fw) //(1)
  priv->fw_dnld.fw->data //(2)|      ...
   ...                        |

The firmware is deallocate in position (1), but it is used in position
(2), which leads to UAF bug.

This patch add spin_lock() and check in nfcmrvl_fw_dnld_start()
in order to synchronize with other threads that could free firmware.
Therefore, the UAF bug could be prevented.

Fixes: 3194c6870158e3 ("NFC: nfcmrvl: add firmware download support")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 drivers/nfc/nfcmrvl/fw_dnld.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/nfc/nfcmrvl/fw_dnld.c b/drivers/nfc/nfcmrvl/fw_dnld.c
index bb9e7e2bdec..910f6eaec65 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -511,7 +511,10 @@ int nfcmrvl_fw_dnld_start(struct nci_dev *ndev, const char *firmware_name)
 		return -ENOENT;
 	}
 
-	fw_dnld->header = (const struct nfcmrvl_fw *) priv->fw_dnld.fw->data;
+	spin_lock(&priv->fw_dnld.lock);
+	if (priv->fw_dnld.fw)
+		fw_dnld->header = (const struct nfcmrvl_fw *)priv->fw_dnld.fw->data;
+	spin_unlock(&priv->fw_dnld.lock);
 
 	if (fw_dnld->header->magic != NFCMRVL_FW_MAGIC ||
 	    fw_dnld->header->phy != priv->phy) {
-- 
2.17.1

Re: [PATCH V2 2/3] drivers: nfc: nfcmrvl: fix double free bug in nfc_fw_download_done()

[Lin Ma]

Hello there,

Sorry, the actual case does not match the description. The netlink operations may has nothing to do with the double free and we will dynamically check this again.

Sorry again for the false tag and the false alarm. T.T

Regards
Lin Ma

Re: [PATCH V3 1/3] drivers: nfc: nfcmrvl: fix double free bugs caused by fw_dnld_over()

[Lin Ma]

Hello there,

Sorry here but actually I didn't "review" this patch but only offer some suggestions.
It seems that the current version of patch mainly focus on solving the data race. However, I'd prefer to make sure this function

> static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)

never be able to be called more than once. Maybe add some additional flag with lock is more appropriate.

Regards
Lin

Re: [PATCH 0/3] Fix double free bugs and UAF bug in nfcmrvl module

[Jakub Kicinski]

On Thu, 14 Apr 2022 13:31:19 +0800 Duoming Zhou wrote:
> We add lock and check in fw_dnld_over() and nfcmrvl_fw_dnld_start(),
> in order to synchronize among different threads that operate on
> firmware.

All the patches must have the same version in the tag.

Also you are CCing a number of people who likely have no interest 
in NFC patches.

Please improve your postings, I've been silently dropping a lot of your
patches because you keep posting them in unusual ways and patchwork is
unable to group them properly :(