LinuxLists.cc - [PATCH 2/2] drivers: nfc: nfcmrvl: fix double free bug in nfc_fw_download

2022-04-12 13:56:35

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

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 <[email protected]>
Reviewed-by: Lin Ma <[email protected]>
---
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

2022-04-12 22:40:16

by kernel test robot

[permalink] [raw]

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

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/[email protected]/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 <[email protected]>

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

2022-04-12 22:41:46

by kernel test robot

[permalink] [raw]