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: <57b9d6c8-557f-fbd7-0069-c84691a76ff4@redhat.com>
Date:   Tue, 1 Aug 2023 16:47:36 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] wifi: brcmfmac: Fix field-spanning write in
 brcmf_scan_params_v2_to_v1()
To:     Kalle Valo <kvalo@kernel.org>
Cc:     Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        SHA-cyfmac-dev-list@infineon.com, Kees Cook <keescook@chromium.org>
References: <20230729140500.27892-1-hdegoede@redhat.com>
 <169090065747.212423.9892152660352726427.kvalo@kernel.org>
Content-Language: en-US
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <169090065747.212423.9892152660352726427.kvalo@kernel.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 8/1/23 16:37, Kalle Valo wrote:
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Using brcmfmac with 6.5-rc3 on a brcmfmac43241b4-sdio triggers
>> a backtrace caused by the following field-spanning error:
>>
>> memcpy: detected field-spanning write (size 120) of single field
>>   "&params_le->channel_list[0]" at
>>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1072 (size 2)
>>
>> Fix this by replacing the channel_list[1] declaration at the end of
>> the struct with a flexible array declaration.
>>
>> Most users of struct brcmf_scan_params_le calculate the size to alloc
>> using the size of the non flex-array part of the struct + needed extra
>> space, so they do not care about sizeof(struct brcmf_scan_params_le).
>>
>> brcmf_notify_escan_complete() however uses the struct on the stack,
>> expecting there to be room for at least 1 entry in the channel-list
>> to store the special -1 abort channel-id.
>>
>> To make this work use an anonymous union with a padding member
>> added + the actual channel_list flexible array.
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> Does the driver still work even if this warning is printed? I'm wondering
> should I take this to wireless or wireless-next. Also a review from Broadcom
> would be really good.

It works fine, but it logs an oops / backtrace.

Note I did test the patch on a device where the warning was triggered and the warning is gone and wifi association still works.

So there is a slight preference to get this as a fix into 6.5 from my side.

> What about a Fixes tag? 

This is caused by the new field-spanning wtire checks enabled recently, so there is not really a brcmfmac commit to point to as the culprit.

Regards,

Hans