wifi: iwlwifi: mvm: Fix -Warray-bounds bug in iwl_mvm_wait_d3_notif()
PATCH v2 1/1 wifi: iwlwifi: mvm: Fix -Warray-bounds bug in iwl_mvm_wait_d3_notif()
kmemdup() at line 2735 is not duplicating enough memory for notif->tid_tear_down and notif->station_id. As it only duplicates 612 bytes: up to offsetofend(struct iwl_wowlan_info_notif, received_beacons), this is the range of [0, 612) bytes.
2735 notif = kmemdup(notif_v1,
2736 offsetofend(struct iwl_wowlan_info_notif,
2737 received_beacons),
2738 GFP_ATOMIC);
which evidently does not cover bytes 612 and 613 for members tid_tear_down and station_id in struct iwl_wowlan_info_notif. See below:
$ pahole -C iwl_wowlan_info_notif drivers/net/wireless/intel/iwlwifi/mvm/d3.o
struct iwl_wowlan_info_notif {
struct iwl_wowlan_gtk_status_v3 gtk[2]; /* 0 488 */
/* --- cacheline 7 boundary (448 bytes) was 40 bytes ago --- */
struct iwl_wowlan_igtk_status igtk[2]; /* 488 80 */
/* --- cacheline 8 boundary (512 bytes) was 56 bytes ago --- */
__le64 replay_ctr; /* 568 8 */
/* --- cacheline 9 boundary (576 bytes) --- */
__le16 pattern_number; /* 576 2 */
__le16 reserved1; /* 578 2 */
__le16 qos_seq_ctr[8]; /* 580 16 */
__le32 wakeup_reasons; /* 596 4 */
__le32 num_of_gtk_rekeys; /* 600 4 */
__le32 transmitted_ndps; /* 604 4 */
__le32 received_beacons; /* 608 4 */
u8 tid_tear_down; /* 612 1 */
u8 station_id; /* 613 1 */
u8 reserved2[2]; /* 614 2 *//* size: 616, cachelines: 10, members: 13 */ /* last cacheline: 40 bytes */ };
Therefore, when the following assignments take place, actually no memory has been allocated for those objects:
2743 notif->tid_tear_down = notif_v1->tid_tear_down;
2744 notif->station_id = notif_v1->station_id;
Fix this by allocating space for the whole notif object and zero out the remaining space in memory after member station_id.
This also fixes the following -Warray-bounds issues:
CC drivers/net/wireless/intel/iwlwifi/mvm/d3.odrivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function ‘iwl_mvm_wait_d3_notif’:drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2743:30: warning: array subscript ‘struct iwl_wowlan_info_notif[0]’ is partly outside array bounds of ‘unsigned char[612]’ [-Warray-bounds=]
2743 | notif->tid_tear_down = notif_v1->tid_tear_down;
|
from drivers/net/wireless/intel/iwlwifi/mvm/d3.c:7:
In function ‘kmemdup’,
inlined from ‘iwl_mvm_wait_d3_notif’ at drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2735:12:
include/linux/fortify-string.h:765:16: note: object of size 612 allocated by ‘__real_kmemdup’
765 | return __real_kmemdup(p, size, gfp);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function ‘iwl_mvm_wait_d3_notif’:drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2744:30: warning: array subscript ‘struct iwl_wowlan_info_notif[0]’ is partly outside array bounds of ‘unsigned char[612]’ [-Warray-bounds=]
2744 | notif->station_id = notif_v1->station_id;
| ^~
In function ‘kmemdup’,
inlined from ‘iwl_mvm_wait_d3_notif’ at drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2735:12:include/linux/fortify-string.h:765:16: note: object of size 612 allocated by ‘__real_kmemdup’
765 | return __real_kmemdup(p, size, gfp);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~---
Changes in v2:
- Use sizeof(*notif), instead of sizeof(struct iwl_wowlan_info_notif).
- Fix typo in the changelog text s/bouds/bounds.
v1:
- Link: https://lore.kernel.org/linux-hardening/ZHpEjTmBys5cCOGZ@work/
drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/net/wireless/intel/iwlwifi/mvm/d3.c +2 -6
@@ -2732,17 +2732,13 @@ static bool iwl_mvm_wait_d3_notif(struct iwl_notif_wait_data *notif_wait,if (wowlan_info_ver <2 ) {struct iwl_wowlan_info_notif_v1 *notif_v1 = (void *)pkt->data; - notif =kmemdup (notif_v1, - offsetofend(struct iwl_wowlan_info_notif, - received_beacons), -GFP_ATOMIC ); - + notif =kmemdup (notif_v1,sizeof (*notif),GFP_ATOMIC );if (!notif)return false; notif->tid_tear_down = notif_v1->tid_tear_down; notif->station_id = notif_v1->station_id; - +memset_after (notif,0 , station_id); }else { notif = (void *)pkt->data; }
Signature · 1 line
2.34.1
Quoted 65 lines · Gustavo A. R. Silva
kmemdup()at line 2735 is not duplicating enough memory fornotif->tid_tear_downandnotif->station_id. As it only duplicates 612 bytes: up tooffsetofend(struct iwl_wowlan_info_notif, received_beacons), this is the range of [0, 612) bytes.2735 notif = kmemdup(notif_v1,
2736 offsetofend(structiwl_wowlan_info_notif,
2737received_beacons),
2738GFP_ATOMIC);which evidently does not cover bytes 612 and 613 for members
tid_tear_downandstation_idin structiwl_wowlan_info_notif. See below:$ pahole -C iwl_wowlan_info_notif drivers/net/wireless/intel/iwlwifi/mvm/d3.o struct iwl_wowlan_info_notif { struct iwl_wowlan_gtk_status_v3 gtk[2]; /* 0 488 */ /* --- cacheline 7 boundary (448 bytes) was 40 bytes ago --- */ struct iwl_wowlan_igtk_status igtk[2]; /* 488 80 */ /* --- cacheline 8 boundary (512 bytes) was 56 bytes ago --- */ __le64 replay_ctr; /* 568 8 */ /* --- cacheline 9 boundary (576 bytes) --- */ __le16 pattern_number; /* 576 2 */ __le16 reserved1; /* 578 2 */ __le16 qos_seq_ctr[8]; /* 580 16 */ __le32 wakeup_reasons; /* 596 4 */ __le32 num_of_gtk_rekeys; /* 600 4 */ __le32 transmitted_ndps; /* 604 4 */ __le32 received_beacons; /* 608 4 */ u8 tid_tear_down; /* 612 1 */ u8 station_id; /* 613 1 */ u8 reserved2[2]; /* 614 2 *//* size: 616, cachelines: 10, members: 13 */ /* last cacheline: 40 bytes */ };Therefore, when the following assignments take place, actually no memory has been allocated for those objects:
2743
notif->tid_tear_down=notif_v1->tid_tear_down;
2744notif->station_id=notif_v1->station_id;Fix this by allocating space for the whole notif object and zero out the remaining space in memory after member
station_id.This also fixes the following -Warray-bounds issues:
CCdrivers/net/wireless/intel/iwlwifi/mvm/d3.odrivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function ‘iwl_mvm_wait_d3_notif’:drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2743:30: warning: array subscript ‘struct iwl_wowlan_info_notif[0]’ is partly outside array bounds of ‘unsigned char[612]’ [-Warray-bounds=]2743 | notif->tid_tear_down = notif_v1->tid_tear_down; | from drivers/net/wireless/intel/iwlwifi/mvm/d3.c:7: In function ‘kmemdup’, inlined from ‘iwl_mvm_wait_d3_notif’ at drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2735:12: include/linux/fortify-string.h:765:16: note: object of size 612 allocated by ‘__real_kmemdup’ 765 | return __real_kmemdup(p, size, gfp); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function ‘iwl_mvm_wait_d3_notif’:drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2744:30: warning: array subscript ‘struct iwl_wowlan_info_notif[0]’ is partly outside array bounds of ‘unsigned char[612]’ [-Warray-bounds=]
2744 |notif->station_id=notif_v1->station_id;
| ^~
In function ‘kmemdup’,
inlined from ‘iwl_mvm_wait_d3_notif’ atdrivers/net/wireless/intel/iwlwifi/mvm/d3.c:2735:12:include/linux/fortify-string.h:765:16: note: object of size 612 allocated by ‘__real_kmemdup’
Quoted 7 lines
765 | return __real_kmemdup(p, size, gfp); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~Fixes 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2")Signed-off-by Gustavo A. R. Silva <[email protected]>
Nice catch!
Quoted 16 lines
---
Changes in v2:
- Usesizeof(*notif), instead ofsizeof(struct iwl_wowlan_info_notif).
- Fix typo in the changelog text s/bouds/bounds.v1:
- Link: https://lore.kernel.org/linux-hardening/ZHpEjTmBys5cCOGZ@work/drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)drivers/net/wireless/intel/iwlwifi/mvm/d3.c +0
-0
drivers/net/wireless/intel/iwlwifi/mvm/d3.c static bool iwl_mvm_wait_d3_notif(struct iwl_notif_wait_data *notif_wait, · line 2735
Quoted 9 lines
drivers/net/wireless/intel/iwlwifi/mvm/d3.c +1
-5@@ -2732,17 +2732,13 @@ static bool iwl_mvm_wait_d3_notif(struct iwl_notif_wait_data *notif_wait,if (wowlan_info_ver <2 ) {struct iwl_wowlan_info_notif_v1 *notif_v1 = (void *)pkt->data; - notif =kmemdup (notif_v1, - offsetofend(struct iwl_wowlan_info_notif, - received_beacons), -GFP_ATOMIC ); - + notif =kmemdup (notif_v1,sizeof (*notif),GFP_ATOMIC );
The only question I have here is whether or not pkt->data actually contains sizeof(*notif)-many bytes? It seems the length isn't checked until after this area:
len = iwl_rx_packet_payload_len(pkt);
So, perhaps this needs to be changed instead, and the length double-checked, etc. Perhaps a regular kzalloc + memcpy is needed to handle pkt->data not being large enough?
Quoted 13 lines
drivers/net/wireless/intel/iwlwifi/mvm/d3.c +1
-1if (!notif)return false; notif->tid_tear_down = notif_v1->tid_tear_down; notif->station_id = notif_v1->station_id; - +memset_after (notif,0 , station_id); }else { notif = (void *)pkt->data; }Signature · 1 line
2.34.1
Signature · 1 line
Kees Cook
Greenman, Gregory
Acked-byQuoted 106 lines · Kees Cook
Quoted 68 lines · Gustavo A. R. Silva
kmemdup()at line 2735 is not duplicating enough memory fornotif->tid_tear_downandnotif->station_id. As it only duplicates 612 bytes: up tooffsetofend(struct iwl_wowlan_info_notif, received_beacons), this is the range of [0, 612) bytes.2735 notif = kmemdup(notif_v1,
2736 offsetofend(structiwl_wowlan_info_notif, 2737received_beacons), 2738 GFP_ATOMIC);which evidently does not cover bytes 612 and 613 for members
tid_tear_downandstation_idin structiwl_wowlan_info_notif. See below:$ pahole -C iwl_wowlan_info_notif drivers/net/wireless/intel/iwlwifi/mvm/d3.o struct iwl_wowlan_info_notif { struct iwl_wowlan_gtk_status_v3 gtk[2]; /* 0 488 */ /* --- cacheline 7 boundary (448 bytes) was 40 bytes ago --- */ struct iwl_wowlan_igtk_status igtk[2]; /* 488 80 */ /* --- cacheline 8 boundary (512 bytes) was 56 bytes ago --- */ __le64 replay_ctr; /* 568 8 */ /* --- cacheline 9 boundary (576 bytes) --- */ __le16 pattern_number; /* 576 2 */ __le16 reserved1; /* 578 2 */ __le16 qos_seq_ctr[8]; /* 580 16 */ __le32 wakeup_reasons; /* 596 4 */ __le32 num_of_gtk_rekeys; /* 600 4 */ __le32 transmitted_ndps; /* 604 4 */ __le32 received_beacons; /* 608 4 */ u8 tid_tear_down; /* 612 1 */ u8 station_id; /* 613 1 */ u8 reserved2[2]; /* 614 2 *//* size: 616, cachelines: 10, members: 13 */ /* last cacheline: 40 bytes */
};Therefore, when the following assignments take place, actually no memory has been allocated for those objects:
2743 notif->tid_tear_down =
notif_v1->tid_tear_down; 2744 notif->station_id =notif_v1->station_id;Fix this by allocating space for the whole notif object and zero out the remaining space in memory after member
station_id.This also fixes the following -Warray-bounds issues:
CCdrivers/net/wireless/intel/iwlwifi/mvm/d3.odrivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function ‘iwl_mvm_wait_d3_notif’:drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2743:30: warning: array subscript ‘struct iwl_wowlan_info_notif[0]’ is partly outside array bounds of ‘unsigned char[612]’ [-Warray-bounds=] 2743 |notif->tid_tear_down=notif_v1->tid_tear_down;
|
fromdrivers/net/wireless/intel/iwlwifi/mvm/d3.c:7:
In function ‘kmemdup’,
inlined from ‘iwl_mvm_wait_d3_notif’ atdrivers/net/wireless/intel/iwlwifi/mvm/d3.c:2735:12:include/linux/fortify-string.h:765:16: note: object of size 612 allocated by ‘__real_kmemdup’
765 | return__real_kmemdup(p, size, gfp);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~drivers/net/wireless/intel/iwlwifi/mvm/d3.c: In function ‘iwl_mvm_wait_d3_notif’:drivers/net/wireless/intel/iwlwifi/mvm/d3.c:2744:30: warning: array subscript ‘struct iwl_wowlan_info_notif[0]’ is partly outside array bounds of ‘unsigned char[612]’ [-Warray-bounds=] 2744 |notif->station_id=notif_v1->station_id;
| ^~
In function ‘kmemdup’,
inlined from ‘iwl_mvm_wait_d3_notif’ atdrivers/net/wireless/intel/iwlwifi/mvm/d3.c:2735:12:include/linux/fortify-string.h:765:16: note: object of size 612 allocated by ‘__real_kmemdup’
765 | return__real_kmemdup(p, size, gfp);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~Quoted 4 lines
Fixes 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2")Signed-off-by Gustavo A. R. Silva <[email protected]>Nice catch!
Quoted 18 lines
---
Changes in v2:
- Usesizeof(*notif), instead ofsizeof(struct iwl_wowlan_info_notif).
- Fix typo in the changelog text s/bouds/bounds.v1:
- Link: https://lore.kernel.org/linux-hardening/ZHpEjTmBys5cCOGZ@work/drivers/net/wireless/intel/iwlwifi/mvm/d3.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)drivers/net/wireless/intel/iwlwifi/mvm/d3.c +0
-0@@ -2732,17 +2732,13 @@ static bool iwl_mvm_wait_d3_notif(struct iwl_notif_wait_data *notif_wait, if (wowlan_info_ver < 2) { struct iwl_wowlan_info_notif_v1 *notif_v1 = (void *)pkt->data;Quoted 7 lines
- notif = kmemdup(notif_v1,
- offsetofend(structiwl_wowlan_info_notif, -received_beacons), - GFP_ATOMIC);
-
+ notif =kmemdup(notif_v1, sizeof(*notif), GFP_ATOMIC);The only question I have here is whether or not
pkt->dataactually contains sizeof(*notif)-many bytes? It seems the length isn't checked until after this area:
Quoted 6 lines
len =
iwl_rx_packet_payload_len(pkt);So, perhaps this needs to be changed instead, and the length double-checked, etc. Perhaps a regular kzalloc + memcpy is needed to handle
pkt->datanot being large enough?
pkt->data always has the correct length as it is the actual data that the firmware sends. The problem is that notif_v1 has two fields in the middle, that notif doesn't have and the purpose of this code is just to remove these two fields, so that the rest of the function could work with notif.
The original code was buggy, since it allocated and copied only the part until the first one of these two fields that we don't need. So, since the fixed code allocates the correct length (and copies the first chunk) with kmemdup() and immediately after sets the fields that come after the "cut out" part, notif will be of correct size and with correct values.
Quoted 5 lines
Quoted 5 lines
if (!notif)
return false;
notif->tid_tear_down =notif_v1->tid_tear_down;notif->station_id=notif_v1->station_id;
Quoted 9 lines
Quoted 8 lines
-
+ memset_after(notif, 0,station_id); } else {
notif = (void *)pkt->data; }Signature · 1 line
2.34.1
Quoted 7 lines · Kees Cook
Quoted 5 lines
Fixes 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2")Signed-off-by Gustavo A. R. Silva <[email protected]>Nice catch!
(dancy-dance):D
,
Quoted 14 lines
drivers/net/wireless/intel/iwlwifi/mvm/d3.c static bool iwl_mvm_wait_d3_notif(struct iwl_notif_wait_data *notif_wait, · line 2735
Quoted 9 lines
if (wowlan_info_ver < 2) { struct iwl_wowlan_info_notif_v1 *notif_v1 = (void *)pkt->data;drivers/net/wireless/intel/iwlwifi/mvm/d3.c +1
-5- notif =kmemdup (notif_v1, - offsetofend(struct iwl_wowlan_info_notif, - received_beacons), -GFP_ATOMIC ); - + notif =kmemdup (notif_v1,sizeof (*notif),GFP_ATOMIC );The only question I have here is whether or not
pkt->dataactually contains sizeof(*notif)-many bytes? It seems the length isn't checked until after this area:
Quoted 5 lines
len = iwl_rx_packet_payload_len(pkt);So, perhaps this needs to be changed instead, and the length double-checked, etc. Perhaps a regular kzalloc + memcpy is needed to handle
pkt->datanot being large enough?
As per Gregory's reply, it seems that the length is just fine.
Thanks
--
Gustavo