* re: wireless: mwifiex: initial commit for Marvell mwifiex driver
@ 2012-04-25 8:44 Dan Carpenter
2012-04-25 22:08 ` Bing Zhao
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2012-04-25 8:44 UTC (permalink / raw)
To: bzhao; +Cc: netdev
Hi Bing,
The patch 5e6e3a92b9a4: "wireless: mwifiex: initial commit for
Marvell mwifiex driver" from Mar 21, 2011, leads to the following
static checker warning:
drivers/net/wireless/mwifiex/sta_ioctl.c:1410
mwifiex_set_gen_ie_helper()
error: memcmp() 'pvendor_ie->oui' too small (3 vs 4)
1390 mwifiex_set_gen_ie_helper(struct mwifiex_private *priv, u8 *ie_data_ptr,
1391 u16 ie_len)
1392 {
1393 int ret = 0;
1394 struct ieee_types_vendor_header *pvendor_ie;
1395 const u8 wpa_oui[] = { 0x00, 0x50, 0xf2, 0x01 };
^^^^^^^^^^^^^^^^^^^^^^^^^^
4 byte array.
1396 const u8 wps_oui[] = { 0x00, 0x50, 0xf2, 0x04 };
1397
1398 /* If the passed length is zero, reset the buffer */
1399 if (!ie_len) {
1400 priv->gen_ie_buf_len = 0;
1401 priv->wps.session_enable = false;
1402
1403 return 0;
1404 } else if (!ie_data_ptr) {
1405 return -1;
1406 }
1407 pvendor_ie = (struct ieee_types_vendor_header *) ie_data_ptr;
1408 /* Test to see if it is a WPA IE, if not, then it is a gen IE */
1409 if (((pvendor_ie->element_id == WLAN_EID_WPA) &&
1410 (!memcmp(pvendor_ie->oui, wpa_oui, sizeof(wpa_oui)))) ||
^^^^^^^^^^^^^^^
->oui is only a 3 byte array so we're reading past the end for this
comparison.
1411 (pvendor_ie->element_id == WLAN_EID_RSN)) {
There are a couple other similar warnings as well:
drivers/net/wireless/mwifiex/sta_ioctl.c:1435 mwifiex_set_gen_ie_helper()
error: memcmp() 'pvendor_ie->oui' too small (3 vs 4)
drivers/net/wireless/mwifiex/scan.c:1177 mwifiex_update_bss_desc_with_ie()
error: memcmp() 'vendor_ie->vend_hdr.oui' too small (3 vs 4)
drivers/net/wireless/mwifiex/scan.c:1185 mwifiex_update_bss_desc_with_ie()
error: memcmp() 'vendor_ie->vend_hdr.oui' too small (3 vs 4)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: wireless: mwifiex: initial commit for Marvell mwifiex driver
2012-04-25 8:44 wireless: mwifiex: initial commit for Marvell mwifiex driver Dan Carpenter
@ 2012-04-25 22:08 ` Bing Zhao
2012-04-26 12:51 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Bing Zhao @ 2012-04-25 22:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: netdev@vger.kernel.org
Hi Dan,
> Hi Bing,
>
> The patch 5e6e3a92b9a4: "wireless: mwifiex: initial commit for
> Marvell mwifiex driver" from Mar 21, 2011, leads to the following
> static checker warning:
>
> drivers/net/wireless/mwifiex/sta_ioctl.c:1410
> mwifiex_set_gen_ie_helper()
> error: memcmp() 'pvendor_ie->oui' too small (3 vs 4)
Thanks for catching this error.
>
> 1390 mwifiex_set_gen_ie_helper(struct mwifiex_private *priv, u8 *ie_data_ptr,
> 1391 u16 ie_len)
> 1392 {
> 1393 int ret = 0;
> 1394 struct ieee_types_vendor_header *pvendor_ie;
> 1395 const u8 wpa_oui[] = { 0x00, 0x50, 0xf2, 0x01 };
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 4 byte array.
The last byte 0x01 is actually the oui_type.
...
u8 oui[3];
u8 oui_type;
...
>
> 1396 const u8 wps_oui[] = { 0x00, 0x50, 0xf2, 0x04 };
> 1397
> 1398 /* If the passed length is zero, reset the buffer */
> 1399 if (!ie_len) {
> 1400 priv->gen_ie_buf_len = 0;
> 1401 priv->wps.session_enable = false;
> 1402
> 1403 return 0;
> 1404 } else if (!ie_data_ptr) {
> 1405 return -1;
> 1406 }
> 1407 pvendor_ie = (struct ieee_types_vendor_header *) ie_data_ptr;
> 1408 /* Test to see if it is a WPA IE, if not, then it is a gen IE */
> 1409 if (((pvendor_ie->element_id == WLAN_EID_WPA) &&
> 1410 (!memcmp(pvendor_ie->oui, wpa_oui, sizeof(wpa_oui)))) ||
> ^^^^^^^^^^^^^^^
> ->oui is only a 3 byte array so we're reading past the end for this
> comparison.
I will fix the misuse of memcmp's.
By the way, could you please share with me how you check for this kind of errors?
Is it coccinelle? I tried with coccicheck but got "spatch.opt unknown option -D" error.
Thanks,
Bing
>
> 1411 (pvendor_ie->element_id == WLAN_EID_RSN)) {
>
>
> There are a couple other similar warnings as well:
>
> drivers/net/wireless/mwifiex/sta_ioctl.c:1435 mwifiex_set_gen_ie_helper()
> error: memcmp() 'pvendor_ie->oui' too small (3 vs 4)
> drivers/net/wireless/mwifiex/scan.c:1177 mwifiex_update_bss_desc_with_ie()
> error: memcmp() 'vendor_ie->vend_hdr.oui' too small (3 vs 4)
> drivers/net/wireless/mwifiex/scan.c:1185 mwifiex_update_bss_desc_with_ie()
> error: memcmp() 'vendor_ie->vend_hdr.oui' too small (3 vs 4)
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: wireless: mwifiex: initial commit for Marvell mwifiex driver
2012-04-25 22:08 ` Bing Zhao
@ 2012-04-26 12:51 ` Dan Carpenter
2012-04-26 19:25 ` Bing Zhao
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2012-04-26 12:51 UTC (permalink / raw)
To: Bing Zhao; +Cc: netdev@vger.kernel.org
On Wed, Apr 25, 2012 at 03:08:33PM -0700, Bing Zhao wrote:
> > ->oui is only a 3 byte array so we're reading past the end for this
> > comparison.
>
> I will fix the misuse of memcmp's.
>
So it wasn't really a bug right? Just a bit confusing.
> By the way, could you please share with me how you check for this kind of errors?
> Is it coccinelle? I tried with coccicheck but got "spatch.opt unknown option -D" error.
>
This was from a Smatch check that I haven't pushed yet.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: wireless: mwifiex: initial commit for Marvell mwifiex driver
2012-04-26 12:51 ` Dan Carpenter
@ 2012-04-26 19:25 ` Bing Zhao
0 siblings, 0 replies; 6+ messages in thread
From: Bing Zhao @ 2012-04-26 19:25 UTC (permalink / raw)
To: Dan Carpenter; +Cc: netdev@vger.kernel.org
Hi Dan,
> On Wed, Apr 25, 2012 at 03:08:33PM -0700, Bing Zhao wrote:
> > > ->oui is only a 3 byte array so we're reading past the end for this
> > > comparison.
> >
> > I will fix the misuse of memcmp's.
> >
>
> So it wasn't really a bug right? Just a bit confusing.
That's right. I will submit a patch to fix the confusion.
>
> > By the way, could you please share with me how you check for this kind of errors?
> > Is it coccinelle? I tried with coccicheck but got "spatch.opt unknown option -D" error.
> >
>
> This was from a Smatch check that I haven't pushed yet.
Are you going to push it soon? I'm eager to use it checking my code.
Thanks,
Bing
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* re: wireless: mwifiex: initial commit for Marvell mwifiex driver
@ 2015-11-24 14:27 Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-11-24 14:27 UTC (permalink / raw)
To: bzhao; +Cc: linux-wireless
Hello Bing Zhao,
The patch 5e6e3a92b9a4: "wireless: mwifiex: initial commit for
Marvell mwifiex driver" from Mar 21, 2011, leads to the following
static checker warning:
drivers/net/wireless/marvell/mwifiex/sta_event.c:634 mwifiex_process_sta_event()
warn: inconsistent indenting
drivers/net/wireless/marvell/mwifiex/sta_event.c
608 case EVENT_PS_AWAKE:
609 mwifiex_dbg(adapter, EVENT, "info: EVENT: AWAKE\n");
610 if (!adapter->pps_uapsd_mode && priv->port_open &&
611 priv->media_connected && adapter->sleep_period.period) {
612 adapter->pps_uapsd_mode = true;
613 mwifiex_dbg(adapter, EVENT,
614 "event: PPS/UAPSD mode activated\n");
615 }
616 adapter->tx_lock_flag = false;
617 if (adapter->pps_uapsd_mode && adapter->gen_null_pkt) {
618 if (mwifiex_check_last_packet_indication(priv)) {
619 if (adapter->data_sent ||
620 (adapter->if_ops.is_port_ready &&
621 !adapter->if_ops.is_port_ready(priv))) {
622 adapter->ps_state = PS_STATE_AWAKE;
623 adapter->pm_wakeup_card_req = false;
624 adapter->pm_wakeup_fw_try = false;
625 del_timer(&adapter->wakeup_timer);
626 break;
627 }
628 if (!mwifiex_send_null_packet
629 (priv,
630 MWIFIEX_TxPD_POWER_MGMT_NULL_PACKET |
631 MWIFIEX_TxPD_POWER_MGMT_LAST_PACKET))
632 adapter->ps_state =
633 PS_STATE_SLEEP;
634 return 0;
The PS_STATE_SLEEP line is indented one tab more than it should be. It
looks like the code is buggy? Possibly curly braces were intended:
if (!mwifiex_send_null_packet(priv,
MWIFIEX_TxPD_POWER_MGMT_NULL_PACKET |
MWIFIEX_TxPD_POWER_MGMT_LAST_PACKET)) {
adapter->ps_state = PS_STATE_SLEEP;
return 0;
}
We're right up against the 80 character limit so it's hard to be sure.
635 }
636 }
637 adapter->ps_state = PS_STATE_AWAKE;
638 adapter->pm_wakeup_card_req = false;
639 adapter->pm_wakeup_fw_try = false;
640 del_timer(&adapter->wakeup_timer);
641
642 break;
643
644 case EVENT_DEEP_SLEEP_AWAKE:
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* re: wireless: mwifiex: initial commit for Marvell mwifiex driver
@ 2015-11-24 14:38 Dan Carpenter
0 siblings, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2015-11-24 14:38 UTC (permalink / raw)
To: bzhao; +Cc: linux-wireless
Hello Bing Zhao,
The patch 5e6e3a92b9a4: "wireless: mwifiex: initial commit for
Marvell mwifiex driver" from Mar 21, 2011, leads to the following
static checker warning:
drivers/net/wireless/marvell/mwifiex/sta_cmd.c:133 mwifiex_cmd_802_11_snmp_mib()
warn: potential memory corrupting cast 2 vs 1 bytes
drivers/net/wireless/marvell/mwifiex/sta_cmd.c
112 static int mwifiex_cmd_802_11_snmp_mib(struct mwifiex_private *priv,
113 struct host_cmd_ds_command *cmd,
114 u16 cmd_action, u32 cmd_oid,
115 u16 *ul_temp)
116 {
117 struct host_cmd_ds_802_11_snmp_mib *snmp_mib = &cmd->params.smib;
118
119 mwifiex_dbg(priv->adapter, CMD,
120 "cmd: SNMP_CMD: cmd_oid = 0x%x\n", cmd_oid);
121 cmd->command = cpu_to_le16(HostCmd_CMD_802_11_SNMP_MIB);
122 cmd->size = cpu_to_le16(sizeof(struct host_cmd_ds_802_11_snmp_mib)
123 - 1 + S_DS_GEN);
124
125 snmp_mib->oid = cpu_to_le16((u16)cmd_oid);
126 if (cmd_action == HostCmd_ACT_GEN_GET) {
127 snmp_mib->query_type = cpu_to_le16(HostCmd_ACT_GEN_GET);
128 snmp_mib->buf_size = cpu_to_le16(MAX_SNMP_BUF_SIZE);
129 le16_add_cpu(&cmd->size, MAX_SNMP_BUF_SIZE);
130 } else if (cmd_action == HostCmd_ACT_GEN_SET) {
131 snmp_mib->query_type = cpu_to_le16(HostCmd_ACT_GEN_SET);
132 snmp_mib->buf_size = cpu_to_le16(sizeof(u16));
133 *((__le16 *) (snmp_mib->value)) = cpu_to_le16(*ul_temp);
^^^^^^^^^^^^^^^
We're writing two bytes here but it's only a 1 byte array. It's a
__packed array as well so there isn't any padding.
134 le16_add_cpu(&cmd->size, sizeof(u16));
135 }
136
137 mwifiex_dbg(priv->adapter, CMD,
138 "cmd: SNMP_CMD: Action=0x%x, OID=0x%x,\t"
139 "OIDSize=0x%x, Value=0x%x\n",
140 cmd_action, cmd_oid, le16_to_cpu(snmp_mib->buf_size),
141 le16_to_cpu(*(__le16 *)snmp_mib->value));
142 return 0;
143 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-11-24 14:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-25 8:44 wireless: mwifiex: initial commit for Marvell mwifiex driver Dan Carpenter
2012-04-25 22:08 ` Bing Zhao
2012-04-26 12:51 ` Dan Carpenter
2012-04-26 19:25 ` Bing Zhao
-- strict thread matches above, loose matches on Subject: below --
2015-11-24 14:27 Dan Carpenter
2015-11-24 14:38 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.