KSPP / linux

Linux kernel source tree (Kernel Self Protection Project)
https://kernsec.org/wiki/index.php/Kernel_Self_Protection_Project
Other
80 stars 5 forks source link

Fix -Warray-bounds warnings in drivers/net/wireless/intel/iwlwifi/mvm/d3.c #306

Closed GustavoARSilva closed 1 year ago

GustavoARSilva commented 1 year ago

 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);
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
GustavoARSilva commented 1 year ago

2723                 struct iwl_wowlan_info_notif *notif;
2724 
2725                 if (d3_data->notif_received & IWL_D3_NOTIF_WOWLAN_INFO) {
2726                         /* We might get two notifications due to dual bss */
2727                         IWL_DEBUG_WOWLAN(mvm,
2728                                          "Got additional wowlan info notification\n");
2729                         break;
2730                 }
2731 
2732                 if (wowlan_info_ver < 2) {
2733                         struct iwl_wowlan_info_notif_v1 *notif_v1 = (void *)pkt->data;
2734 
2735                         notif = kmemdup(notif_v1,
2736                                         offsetofend(struct iwl_wowlan_info_notif,
2737                                                     received_beacons),
2738                                         GFP_ATOMIC);
2739 
2740                         if (!notif)
2741                                 return false;
2742 
2743                         notif->tid_tear_down = notif_v1->tid_tear_down;
2744                         notif->station_id = notif_v1->station_id;

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), 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;
GustavoARSilva commented 1 year ago

We need to allocate memory for the whole notif object. This should fix this problem:


diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
index 37aa4676dc94..2760b5fc7e0b 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/d3.c
@@ -2733,8 +2733,7 @@ static bool iwl_mvm_wait_d3_notif(struct iwl_notif_wait_data *notif_wait,
                        struct iwl_wowlan_info_notif_v1 *notif_v1 = (void *)pkt->data;

                        notif = kmemdup(notif_v1,
-                                       offsetofend(struct iwl_wowlan_info_notif,
-                                                   received_beacons),
+                                       sizeof(struct iwl_wowlan_info_notif),
                                        GFP_ATOMIC);

                        if (!notif)
GustavoARSilva commented 1 year ago

Patch: https://lore.kernel.org/linux-hardening/ZHpGN555FwAKGduH@work/