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@xxxxxxxxxxxxxxx
> Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
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
>
--
Kees Cook
