Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <62ed7ac9-71bf-1fa5-1ae3-9c2060393b4b@embeddedor.com>
Date:   Tue, 6 Jun 2023 11:36:14 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2] wifi: iwlwifi: mvm: Fix -Warray-bounds bug in
 iwl_mvm_wait_d3_notif()
Content-Language: en-US
To:     Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc:     Gregory Greenman <gregory.greenman@intel.com>,
        Kalle Valo <kvalo@kernel.org>,
        Haim Dreyfuss <haim.dreyfuss@intel.com>,
        Johannes Berg <johannes.berg@intel.com>,
        linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
References: <ZHpGN555FwAKGduH@work> <202306051758.CD86F1E638@keescook>
From:   "Gustavo A. R. Silva" <gustavo@embeddedor.com>
In-Reply-To: <202306051758.CD86F1E638@keescook>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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