[PATCH 0/2] Fix double free bugs in nfcmrvl module

[PATCH 0/2] Fix double free bugs in nfcmrvl module

[Duoming Zhou]

We add lock and check in fw_dnld_over() in order to synchronize
among different threads that call fw_dnld_over(). The first patch
fixes double free bugs caused by fw_dnld_over() and the second patch
fixes double free bug in nfc_fw_download_done().

Duoming Zhou (2):
  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/fw_dnld.c | 8 +++++++-
 drivers/nfc/nfcmrvl/fw_dnld.h | 2 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH V3 1/2] 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..e4fe0fb8aff 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 fw_dnld_over.*/
+	spinlock_t lock;
 };
 
 int nfcmrvl_fw_dnld_init(struct nfcmrvl_private *priv);
-- 
2.17.1

[PATCH 2/2] 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>
---
 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 c22a4556db5..7586a2f678d 100644
--- a/drivers/nfc/nfcmrvl/fw_dnld.c
+++ b/drivers/nfc/nfcmrvl/fw_dnld.c
@@ -116,7 +116,10 @@ static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
 		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 (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

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

[kernel test robot]

Hi Duoming,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master linux/master v5.18-rc2 next-20220412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Duoming-Zhou/Fix-double-free-bugs-in-nfcmrvl-module/20220412-203028
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b66bfc131c69bd9a5ed3ae90be4cf47ec46c1526
config: i386-randconfig-a004-20220411 (https://download.01.org/0day-ci/archive/20220413/202204130040.8kwehlO8-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project fe2478d44e4f7f191c43fef629ac7a23d0251e72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1f4dba76cb2e854d8ae29781d066257f58b33dee
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Duoming-Zhou/Fix-double-free-bugs-in-nfcmrvl-module/20220412-203028
        git checkout 1f4dba76cb2e854d8ae29781d066257f58b33dee
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/nfc/nfcmrvl/fw_dnld.c:120:6: error: use of undeclared identifier 'dev'
           if (dev->fw_download_in_progress)
               ^
   1 error generated.


vim +/dev +120 drivers/nfc/nfcmrvl/fw_dnld.c

    92	
    93	static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
    94	{
    95		spin_lock_irq(&priv->fw_dnld.lock);
    96		if (priv->fw_dnld.fw) {
    97			release_firmware(priv->fw_dnld.fw);
    98			priv->fw_dnld.fw = NULL;
    99			priv->fw_dnld.header = NULL;
   100			priv->fw_dnld.binary_config = NULL;
   101		}
   102		spin_unlock_irq(&priv->fw_dnld.lock);
   103	
   104		atomic_set(&priv->ndev->cmd_cnt, 0);
   105	
   106		if (timer_pending(&priv->ndev->cmd_timer))
   107			del_timer_sync(&priv->ndev->cmd_timer);
   108	
   109		if (timer_pending(&priv->fw_dnld.timer))
   110			del_timer_sync(&priv->fw_dnld.timer);
   111	
   112		nfc_info(priv->dev, "FW loading over (%d)]\n", error);
   113	
   114		if (error != 0) {
   115			/* failed, halt the chip to avoid power consumption */
   116			nfcmrvl_chip_halt(priv);
   117		}
   118	
   119		spin_lock_irq(&priv->fw_dnld.lock);
 > 120		if (dev->fw_download_in_progress)
   121			nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
   122		spin_unlock_irq(&priv->fw_dnld.lock);
   123	}
   124	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Duoming,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master linux/master v5.18-rc2 next-20220412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Duoming-Zhou/Fix-double-free-bugs-in-nfcmrvl-module/20220412-203028
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b66bfc131c69bd9a5ed3ae90be4cf47ec46c1526
config: openrisc-randconfig-r033-20220411 (https://download.01.org/0day-ci/archive/20220413/202204130213.ukrJxJpy-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1f4dba76cb2e854d8ae29781d066257f58b33dee
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Duoming-Zhou/Fix-double-free-bugs-in-nfcmrvl-module/20220412-203028
        git checkout 1f4dba76cb2e854d8ae29781d066257f58b33dee
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/nfc/nfcmrvl/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/build_bug.h:5,
                    from include/linux/container_of.h:5,
                    from include/linux/list.h:5,
                    from include/linux/module.h:12,
                    from drivers/nfc/nfcmrvl/fw_dnld.c:8:
   drivers/nfc/nfcmrvl/fw_dnld.c: In function 'fw_dnld_over':
>> drivers/nfc/nfcmrvl/fw_dnld.c:120:13: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
     120 |         if (dev->fw_download_in_progress)
         |             ^~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   drivers/nfc/nfcmrvl/fw_dnld.c:120:9: note: in expansion of macro 'if'
     120 |         if (dev->fw_download_in_progress)
         |         ^~
   drivers/nfc/nfcmrvl/fw_dnld.c:120:13: note: each undeclared identifier is reported only once for each function it appears in
     120 |         if (dev->fw_download_in_progress)
         |             ^~~
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
      58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
         |                                                    ^~~~
   drivers/nfc/nfcmrvl/fw_dnld.c:120:9: note: in expansion of macro 'if'
     120 |         if (dev->fw_download_in_progress)
         |         ^~


vim +120 drivers/nfc/nfcmrvl/fw_dnld.c

   > 8	#include <linux/module.h>
     9	#include <asm/unaligned.h>
    10	#include <linux/firmware.h>
    11	#include <linux/nfc.h>
    12	#include <net/nfc/nci.h>
    13	#include <net/nfc/nci_core.h>
    14	#include "nfcmrvl.h"
    15	
    16	#define FW_DNLD_TIMEOUT			15000
    17	
    18	#define NCI_OP_PROPRIETARY_BOOT_CMD	nci_opcode_pack(NCI_GID_PROPRIETARY, \
    19								NCI_OP_PROP_BOOT_CMD)
    20	
    21	/* FW download states */
    22	
    23	enum {
    24		STATE_RESET = 0,
    25		STATE_INIT,
    26		STATE_SET_REF_CLOCK,
    27		STATE_SET_HI_CONFIG,
    28		STATE_OPEN_LC,
    29		STATE_FW_DNLD,
    30		STATE_CLOSE_LC,
    31		STATE_BOOT
    32	};
    33	
    34	enum {
    35		SUBSTATE_WAIT_COMMAND = 0,
    36		SUBSTATE_WAIT_ACK_CREDIT,
    37		SUBSTATE_WAIT_NACK_CREDIT,
    38		SUBSTATE_WAIT_DATA_CREDIT,
    39	};
    40	
    41	/*
    42	 * Patterns for responses
    43	 */
    44	
    45	static const uint8_t nci_pattern_core_reset_ntf[] = {
    46		0x60, 0x00, 0x02, 0xA0, 0x01
    47	};
    48	
    49	static const uint8_t nci_pattern_core_init_rsp[] = {
    50		0x40, 0x01, 0x11
    51	};
    52	
    53	static const uint8_t nci_pattern_core_set_config_rsp[] = {
    54		0x40, 0x02, 0x02, 0x00, 0x00
    55	};
    56	
    57	static const uint8_t nci_pattern_core_conn_create_rsp[] = {
    58		0x40, 0x04, 0x04, 0x00
    59	};
    60	
    61	static const uint8_t nci_pattern_core_conn_close_rsp[] = {
    62		0x40, 0x05, 0x01, 0x00
    63	};
    64	
    65	static const uint8_t nci_pattern_core_conn_credits_ntf[] = {
    66		0x60, 0x06, 0x03, 0x01, NCI_CORE_LC_CONNID_PROP_FW_DL, 0x01
    67	};
    68	
    69	static const uint8_t nci_pattern_proprietary_boot_rsp[] = {
    70		0x4F, 0x3A, 0x01, 0x00
    71	};
    72	
    73	static struct sk_buff *alloc_lc_skb(struct nfcmrvl_private *priv, uint8_t plen)
    74	{
    75		struct sk_buff *skb;
    76		struct nci_data_hdr *hdr;
    77	
    78		skb = nci_skb_alloc(priv->ndev, (NCI_DATA_HDR_SIZE + plen), GFP_KERNEL);
    79		if (!skb)
    80			return NULL;
    81	
    82		hdr = skb_put(skb, NCI_DATA_HDR_SIZE);
    83		hdr->conn_id = NCI_CORE_LC_CONNID_PROP_FW_DL;
    84		hdr->rfu = 0;
    85		hdr->plen = plen;
    86	
    87		nci_mt_set((__u8 *)hdr, NCI_MT_DATA_PKT);
    88		nci_pbf_set((__u8 *)hdr, NCI_PBF_LAST);
    89	
    90		return skb;
    91	}
    92	
    93	static void fw_dnld_over(struct nfcmrvl_private *priv, u32 error)
    94	{
    95		spin_lock_irq(&priv->fw_dnld.lock);
    96		if (priv->fw_dnld.fw) {
    97			release_firmware(priv->fw_dnld.fw);
    98			priv->fw_dnld.fw = NULL;
    99			priv->fw_dnld.header = NULL;
   100			priv->fw_dnld.binary_config = NULL;
   101		}
   102		spin_unlock_irq(&priv->fw_dnld.lock);
   103	
   104		atomic_set(&priv->ndev->cmd_cnt, 0);
   105	
   106		if (timer_pending(&priv->ndev->cmd_timer))
   107			del_timer_sync(&priv->ndev->cmd_timer);
   108	
   109		if (timer_pending(&priv->fw_dnld.timer))
   110			del_timer_sync(&priv->fw_dnld.timer);
   111	
   112		nfc_info(priv->dev, "FW loading over (%d)]\n", error);
   113	
   114		if (error != 0) {
   115			/* failed, halt the chip to avoid power consumption */
   116			nfcmrvl_chip_halt(priv);
   117		}
   118	
   119		spin_lock_irq(&priv->fw_dnld.lock);
 > 120		if (dev->fw_download_in_progress)
   121			nfc_fw_download_done(priv->ndev->nfc_dev, priv->fw_dnld.name, error);
   122		spin_unlock_irq(&priv->fw_dnld.lock);
   123	}
   124	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp