diff mbox series

[v2] wifi: iwlwifi: mvm: Fix -Warray-bounds bug in iwl_mvm_wait_d3_notif()

Message ID ZHpGN555FwAKGduH@work
State New
Headers
Series [v2] wifi: iwlwifi: mvm: Fix -Warray-bounds bug in iwl_mvm_wait_d3_notif() |

Commit Message

Gustavo A. R. Silva June 2, 2023, 7:42 p.m. UTC 
  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(-)

Comments

Kees Cook June 6, 2023, 1:03 a.m. UTC | #1 
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
>
Greenman, Gregory June 6, 2023, 9:09 a.m. UTC | #2 
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>
Gustavo A. R. Silva June 6, 2023, 5:36 p.m. UTC | #3 
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 mbox series

Patch

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;
 		}