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: <80b22368-718b-a7d8-eb01-9bb7b4d22742@redhat.com>
Date:   Tue, 18 Apr 2023 17:28:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH] wifi: iwlwifi: dvm: Fix memcpy: detected field-spanning
 write backtrace
To:     Johannes Berg <johannes@sipsolutions.net>,
        Gregory Greenman <gregory.greenman@intel.com>,
        Kalle Valo <kvalo@kernel.org>
Cc:     linux-wireless@vger.kernel.org, Kees Cook <keescook@chromium.org>
References: <20230418132546.45297-1-hdegoede@redhat.com>
 <9619692082caef649e236eb154129e922c46b547.camel@sipsolutions.net>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <9619692082caef649e236eb154129e922c46b547.camel@sipsolutions.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 4/18/23 16:43, Johannes Berg wrote:
> On Tue, 2023-04-18 at 15:25 +0200, Hans de Goede wrote:
>> A received TKIP key may be up to 32 bytes because it may contain
>> MIC rx/tx keys too. These are not used by iwl and copying these
>> over overflows the iwl_keyinfo.key field.
> 
> Thanks for doing (and more importantly testing) this :)
> 
>> -		memcpy(sta_cmd.key.key, keyconf->key, keyconf->keylen);
>> +		/* keyconf may contain MIC rx/tx keys which iwl does not use */
>> +		to_copy = min_t(size_t, sizeof(sta_cmd.key.key), keyconf->keylen);
>> +		memcpy(sta_cmd.key.key, keyconf->key, to_copy);
>>
> 
> I'd kind of argue that just hardcoding 16 is fine here, keylen _looks_
> variable so the compiler can't optimize it away, but in reality keylen
> always must be 32 and sizeof() must be always 16 :)

It is not just the compiler to whom  keylen _looks_ variable,
so to be safe I went with the min_t() check. I think this
also makes the code easier to understand for future readers of
the code.

> But it also really doesn't matter.
> 
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

Thanks.

> Should we Cc stable on this?

Yes I think so. I did not add the tag because some subsystems
want that to be left up to the maintainer.

Regards,

Hans