| Message ID | ZHpGN555FwAKGduH@work (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v2] wifi: iwlwifi: mvm: Fix -Warray-bounds bug in iwl_mvm_wait_d3_notif() | expand |
On Fri, Jun 02, 2023 at 01:42:47PM -0600, Gustavo A. R. Silva wrote: > 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.o > drivers/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); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Link: https://github.com/KSPP/linux/issues/306 > Fixes: 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2") > Cc: stable@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> Nice catch! > --- > 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(-) > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > index 37aa4676dc94..6d1007f24b4a 100644 > --- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > @@ -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? > 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; > } > -- > 2.34.1 >
On Mon, 2023-06-05 at 18:03 -0700, Kees Cook wrote: > On Fri, Jun 02, 2023 at 01:42:47PM -0600, Gustavo A. R. Silva wrote: > > 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.o > > drivers/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); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Link: https://github.com/KSPP/linux/issues/306 > > Fixes: 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2") > > Cc: stable@vger.kernel.org > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > Nice catch! > > > --- > > 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(-) > > > > diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > > index 37aa4676dc94..6d1007f24b4a 100644 > > --- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > > +++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c > > @@ -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? > 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. > > 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; > > } > > -- > > 2.34.1 > > > Acked-by: Gregory Greenman <gregory.greenman@intel.com>
On 6/5/23 19:03, Kees Cook wrote: >> >> Link: https://github.com/KSPP/linux/issues/306 >> Fixes: 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2") >> Cc: stable@vger.kernel.org >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > Nice catch! (dancy-dance):D , >> 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? As per Gregory's reply, it seems that the length is just fine. Thanks -- Gustavo
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c index 37aa4676dc94..6d1007f24b4a 100644 --- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c +++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c @@ -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; }
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.o drivers/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); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ Link: https://github.com/KSPP/linux/issues/306 Fixes: 905d50ddbc83 ("wifi: iwlwifi: mvm: support wowlan info notification version 2") Cc: stable@vger.kernel.org Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- 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(-)