Prefer 'strscpy()' over 'strlcpy()' in 'mwifiex_init_hw_fw()'.
Signed-off-by: Dmitry Antipov <[email protected]>
---
v4: simplify to drop strlcpy() only (Brian Norris)
---
drivers/net/wireless/marvell/mwifiex/main.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index ea22a08e6c08..64512b00e8b5 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -724,14 +724,9 @@ static int mwifiex_init_hw_fw(struct mwifiex_adapter *adapter,
/* Override default firmware with manufacturing one if
* manufacturing mode is enabled
*/
- if (mfg_mode) {
- if (strlcpy(adapter->fw_name, MFG_FIRMWARE,
- sizeof(adapter->fw_name)) >=
- sizeof(adapter->fw_name)) {
- pr_err("%s: fw_name too long!\n", __func__);
- return -1;
- }
- }
+ if (mfg_mode)
+ strscpy(adapter->fw_name, MFG_FIRMWARE,
+ sizeof(adapter->fw_name));
if (req_fw_nowait) {
ret = request_firmware_nowait(THIS_MODULE, 1, adapter->fw_name,
--
2.41.0
When compiling with gcc 13.1 and CONFIG_FORTIFY_SOURCE=y,
I've noticed the following:
In function ‘fortify_memcpy_chk’,
inlined from ‘mwifiex_construct_tdls_action_frame’ at drivers/net/wireless/marvell/mwifiex/tdls.c:765:3,
inlined from ‘mwifiex_send_tdls_action_frame’ at drivers/net/wireless/marvell/mwifiex/tdls.c:856:6:
./include/linux/fortify-string.h:529:25: warning: call to ‘__read_overflow2_field’
declared with attribute warning: detected read beyond size of field (2nd parameter);
maybe use struct_group()? [-Wattribute-warning]
529 | __read_overflow2_field(q_size_field, size);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
The compiler actually complains on:
memmove(pos + ETH_ALEN, &mgmt->u.action.category,
sizeof(mgmt->u.action.u.tdls_discover_resp));
and it happens because the fortification logic interprets this
as an attempt to overread 1-byte 'u.action.category' member of
'struct ieee80211_mgmt'. To silence this warning, it's enough
to pass an address of 'u.action' itself instead of an address
of its first member.
This also fixes an improper usage of 'sizeof()'. Since 'skb' is
extended with 'sizeof(mgmt->u.action.u.tdls_discover_resp) + 1'
bytes (where 1 is actually 'sizeof(mgmt->u.action.category)'),
I assume that the same number of bytes should be copied.
Suggested-by: Brian Norris <[email protected]>
Signed-off-by: Dmitry Antipov <[email protected]>
---
v4: fix memmove() size calculation (Brian Norris)
---
drivers/net/wireless/marvell/mwifiex/tdls.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 97bb87c3676b..6c60621b6ccc 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -735,6 +735,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
int ret;
u16 capab;
struct ieee80211_ht_cap *ht_cap;
+ unsigned int extra;
u8 radio, *pos;
capab = priv->curr_bss_params.bss_descriptor.cap_info_bitmap;
@@ -753,7 +754,10 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
switch (action_code) {
case WLAN_PUB_ACTION_TDLS_DISCOVER_RES:
- skb_put(skb, sizeof(mgmt->u.action.u.tdls_discover_resp) + 1);
+ /* See the layout of 'struct ieee80211_mgmt'. */
+ extra = sizeof(mgmt->u.action.u.tdls_discover_resp) +
+ sizeof(mgmt->u.action.category);
+ skb_put(skb, extra);
mgmt->u.action.category = WLAN_CATEGORY_PUBLIC;
mgmt->u.action.u.tdls_discover_resp.action_code =
WLAN_PUB_ACTION_TDLS_DISCOVER_RES;
@@ -762,8 +766,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
mgmt->u.action.u.tdls_discover_resp.capability =
cpu_to_le16(capab);
/* move back for addr4 */
- memmove(pos + ETH_ALEN, &mgmt->u.action.category,
- sizeof(mgmt->u.action.u.tdls_discover_resp));
+ memmove(pos + ETH_ALEN, &mgmt->u.action, extra);
/* init address 4 */
eth_broadcast_addr(pos);
--
2.41.0
Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
'mwifiex_process_uap_txpd()'. In case of insufficient
headrom, issue warning and return NULL, which should be
gracefully handled in 'mwifiex_process_tx()'. Also mark
error handling branches with 'unlikely()' and adjust
format specifiers to match actual 'unsigned int' type.
Signed-off-by: Dmitry Antipov <[email protected]>
---
v4: initial version to match series
---
drivers/net/wireless/marvell/mwifiex/sta_tx.c | 13 +++++++++----
drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++----
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
index 13c0e67ededf..d43f6ec1ad37 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
@@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
u16 pkt_type, pkt_offset;
int hroom = adapter->intf_hdr_len;
- if (!skb->len) {
+ if (unlikely(!skb->len)) {
mwifiex_dbg(adapter, ERROR,
- "Tx: bad packet length: %d\n", skb->len);
+ "Tx: bad packet length: %u\n", skb->len);
tx_info->status_code = -1;
return skb->data;
}
-
- BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
+ if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
+ mwifiex_dbg(adapter, ERROR,
+ "Tx: insufficient skb headroom: %u\n",
+ skb_headroom(skb));
+ tx_info->status_code = -1;
+ return NULL;
+ }
pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
index e495f7eaea03..b27266742795 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
@@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
u16 pkt_type, pkt_offset;
int hroom = adapter->intf_hdr_len;
- if (!skb->len) {
+ if (unlikely(!skb->len)) {
mwifiex_dbg(adapter, ERROR,
- "Tx: bad packet length: %d\n", skb->len);
+ "Tx: bad packet length: %u\n", skb->len);
tx_info->status_code = -1;
return skb->data;
}
-
- BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
+ if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
+ mwifiex_dbg(adapter, ERROR,
+ "Tx: insufficient skb headroom: %u\n",
+ skb_headroom(skb));
+ tx_info->status_code = -1;
+ return NULL;
+ }
pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
--
2.41.0
On Thu, Jun 29, 2023 at 11:51:02AM +0300, Dmitry Antipov wrote:
> Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
> 'mwifiex_process_uap_txpd()'. In case of insufficient
> headrom, issue warning and return NULL, which should be
Hi Dimitry,
a minor nit courtesy of checkpatch.pl --codespell: headrom -> headroom
...
On Thu, Jun 29, 2023 at 11:51:00AM +0300, Dmitry Antipov wrote:
> Prefer 'strscpy()' over 'strlcpy()' in 'mwifiex_init_hw_fw()'.
>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> v4: simplify to drop strlcpy() only (Brian Norris)
Reviewed-by: Brian Norris <[email protected]>
On Thu, Jun 29, 2023 at 11:51:01AM +0300, Dmitry Antipov wrote:
[...]
> This also fixes an improper usage of 'sizeof()'. Since 'skb' is
> extended with 'sizeof(mgmt->u.action.u.tdls_discover_resp) + 1'
> bytes (where 1 is actually 'sizeof(mgmt->u.action.category)'),
> I assume that the same number of bytes should be copied.
>
> Suggested-by: Brian Norris <[email protected]>
I don't believe I actually *suggested* the change; I just highlighted
that the size looked sketchy in the original code. :)
But your change does look correct, and I don't see how we could possibly
*want* to be off by 1 here, so:
Reviewed-by: Brian Norris <[email protected]>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> v4: fix memmove() size calculation (Brian Norris)
> ---
> drivers/net/wireless/marvell/mwifiex/tdls.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
On Thu, Jun 29, 2023 at 11:51:02AM +0300, Dmitry Antipov wrote:
> Remove 'BUG_ON()' from 'mwifiex_process_sta_txpd()' and
> 'mwifiex_process_uap_txpd()'. In case of insufficient
> headrom, issue warning and return NULL, which should be
> gracefully handled in 'mwifiex_process_tx()'. Also mark
> error handling branches with 'unlikely()' and adjust
> format specifiers to match actual 'unsigned int' type.
>
> Signed-off-by: Dmitry Antipov <[email protected]>
> ---
> v4: initial version to match series
> ---
> drivers/net/wireless/marvell/mwifiex/sta_tx.c | 13 +++++++++----
> drivers/net/wireless/marvell/mwifiex/uap_txrx.c | 13 +++++++++----
> 2 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_tx.c b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> index 13c0e67ededf..d43f6ec1ad37 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_tx.c
> @@ -39,14 +39,19 @@ void *mwifiex_process_sta_txpd(struct mwifiex_private *priv,
> u16 pkt_type, pkt_offset;
> int hroom = adapter->intf_hdr_len;
>
> - if (!skb->len) {
> + if (unlikely(!skb->len)) {
> mwifiex_dbg(adapter, ERROR,
> - "Tx: bad packet length: %d\n", skb->len);
> + "Tx: bad packet length: %u\n", skb->len);
> tx_info->status_code = -1;
> return skb->data;
> }
> -
> - BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
> + if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
> + mwifiex_dbg(adapter, ERROR,
> + "Tx: insufficient skb headroom: %u\n",
> + skb_headroom(skb));
> + tx_info->status_code = -1;
> + return NULL;
I'm not sure why this return (NULL) should be different than the one for
skb->len==0 (skb->data). mwifiex_process_tx() has...weird handling for
both.
For NULL, we fall into a default (ret==-1) case where the error message
is wrong ("mwifiex_write_data_async failed: ...").
For non-NULL skb->data, we still try to queue or transmit the
skb...which seems wrong.
I think they should both be returning NULL, and mwifiex_process_tx()
should improve its error handling to more explicitly handle that case,
instead of printing the wrong error message.
(Now, I expect neither failure cases are actually exercised in practice,
which makes most of this moot...)
I'm also not sure why this is part of the same series as the others.
Brian
> + }
>
> pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> index e495f7eaea03..b27266742795 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_txrx.c
> @@ -452,14 +452,19 @@ void *mwifiex_process_uap_txpd(struct mwifiex_private *priv,
> u16 pkt_type, pkt_offset;
> int hroom = adapter->intf_hdr_len;
>
> - if (!skb->len) {
> + if (unlikely(!skb->len)) {
> mwifiex_dbg(adapter, ERROR,
> - "Tx: bad packet length: %d\n", skb->len);
> + "Tx: bad packet length: %u\n", skb->len);
> tx_info->status_code = -1;
> return skb->data;
> }
> -
> - BUG_ON(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN);
> + if (unlikely(skb_headroom(skb) < MWIFIEX_MIN_DATA_HEADER_LEN)) {
> + mwifiex_dbg(adapter, ERROR,
> + "Tx: insufficient skb headroom: %u\n",
> + skb_headroom(skb));
> + tx_info->status_code = -1;
> + return NULL;
> + }
>
> pkt_type = mwifiex_is_skb_mgmt_frame(skb) ? PKT_TYPE_MGMT : 0;
>
> --
> 2.41.0
>
Dmitry Antipov <[email protected]> wrote:
> Prefer 'strscpy()' over 'strlcpy()' in 'mwifiex_init_hw_fw()'.
>
> Signed-off-by: Dmitry Antipov <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
2 patches applied to wireless-next.git, thanks.
caf9ead2c7d0 wifi: mwifiex: prefer strscpy() over strlcpy()
dcce94b80a95 wifi: mwifiex: fix fortify warning
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches