2014-02-14 09:02:56

by Dan Carpenter

Subject: re: mwifiex: parse TDLS action frames during RX

Hello Avinash Patil,

The patch 5f2caaf32bc6: "mwifiex: parse TDLS action frames during RX"
from Feb 7, 2014, leads to the following static checker warning:

drivers/net/wireless/mwifiex/tdls.c:820 mwifiex_process_tdls_action_frame()
error: memcpy() '&sta_ptr->tdls_cap.rsn_ie' too small (256 vs 257)

drivers/net/wireless/mwifiex/tdls.c
814 case WLAN_EID_EXT_CAPABILITY:
815 memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos,
816 sizeof(struct ieee_types_header) +
817 min_t(u8, pos[1], 8));
818 break;
819 case WLAN_EID_RSN:
820 memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos,
821 sizeof(struct ieee_types_header) + pos[1]);

Smatch thinks pos[] is untrusted data because it comes from skb->data
in mwifiex_process_rx_packet().

sta_ptr->tdls_cap.rsn_ie is defined like:

struct ieee_types_generic {
struct ieee_types_header ieee_hdr;
u8 data[IEEE_MAX_IE_SIZE - sizeof(struct ieee_types_header)];
} __packed;

So it is IEEE_MAX_IE_SIZE (256) bytes long. Meanwhile the memcpy()
limit is 2 + 0xff, so it's 257 and we are corrupting a byte past the end
of the struct.

822 break;
823 case WLAN_EID_QOS_CAPA:
824 sta_ptr->tdls_cap.qos_info = pos[2];
825 break;

regards,
dan carpenter

2014-02-14 09:57:30

by Avinash Patil

Subject: RE: mwifiex: parse TDLS action frames during RX

Hi Dan,

Thanks for reporting the issue.

I will submit a patch to fix this warning.

Thanks and Regards,
Avinash Patil

-----Original Message-----
From: Dan Carpenter [mailto:[email protected]]
Sent: Friday, February 14, 2014 2:33 PM
To: Avinash Patil
Cc: [email protected]; Bing Zhao
Subject: re: mwifiex: parse TDLS action frames during RX

Hello Avinash Patil,

The patch 5f2caaf32bc6: "mwifiex: parse TDLS action frames during RX"
from Feb 7, 2014, leads to the following static checker warning:

drivers/net/wireless/mwifiex/tdls.c:820 mwifiex_process_tdls_action_frame()
error: memcpy() '&sta_ptr->tdls_cap.rsn_ie' too small (256 vs 257)

drivers/net/wireless/mwifiex/tdls.c
814 case WLAN_EID_EXT_CAPABILITY:
815 memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos,
816 sizeof(struct ieee_types_header) +
817 min_t(u8, pos[1], 8));
818 break;
819 case WLAN_EID_RSN:
820 memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos,
821 sizeof(struct ieee_types_header) + pos[1]);

Smatch thinks pos[] is untrusted data because it comes from skb->data
in mwifiex_process_rx_packet().

sta_ptr->tdls_cap.rsn_ie is defined like:

struct ieee_types_generic {
struct ieee_types_header ieee_hdr;
u8 data[IEEE_MAX_IE_SIZE - sizeof(struct ieee_types_header)];
} __packed;

So it is IEEE_MAX_IE_SIZE (256) bytes long. Meanwhile the memcpy()
limit is 2 + 0xff, so it's 257 and we are corrupting a byte past the end
of the struct.

822 break;
823 case WLAN_EID_QOS_CAPA:
824 sta_ptr->tdls_cap.qos_info = pos[2];
825 break;

regards,
dan carpenter