| Message ID | 20230729140500.27892-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: brcmfmac: Fix field-spanning write in brcmf_scan_params_v2_to_v1() | expand |
On Sat, Jul 29, 2023 at 04:05:00PM +0200, Hans de Goede 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 > "¶ms_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> Looks good to me; it's consistent with how similar 1-element arrays with sensitive structure sizes have been updated lately. Thanks for the investigation! Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
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 > "¶ms_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. What about a Fixes tag?
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 >> "¶ms_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
Hans de Goede <hdegoede@redhat.com> writes: > 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 >>> "¶ms_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. I'll add that info to the commit log. > 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. I'll queue this wireless then. But I really would like to Broadcom take a look at this in case we are missing something. >> 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. Ok.
On Tue, Aug 1, 2023 at 7:55 AM Kalle Valo <kvalo@kernel.org> wrote: > > Hans de Goede <hdegoede@redhat.com> writes: > > > 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 > >>> "¶ms_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> Looks good to me, thanks for taking care of it. Reviewed-by: Franky Lin <franky.lin@broadcom.com>
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 warning: > > memcpy: detected field-spanning write (size 120) of single field > "¶ms_le->channel_list[0]" at > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1072 (size 2) > > The driver still works after this warning. The warning was introduced by the > new field-spanning write checks which were enabled recently. > > 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> > Reviewed-by: Franky Lin <franky.lin@broadcom.com> Patch applied to wireless.git, thanks. 16e455a465fc wifi: brcmfmac: Fix field-spanning write in brcmf_scan_params_v2_to_v1()
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h index 792adaf880b4..bece26741d3a 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h @@ -398,7 +398,12 @@ struct brcmf_scan_params_le { * fixed parameter portion is assumed, otherwise * ssid in the fixed portion is ignored */ - __le16 channel_list[1]; /* list of chanspecs */ + union { + __le16 padding; /* Reserve space for at least 1 entry for abort + * which uses an on stack brcmf_scan_params_le + */ + DECLARE_FLEX_ARRAY(__le16, channel_list); /* chanspecs */ + }; }; struct brcmf_scan_params_v2_le {
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 "¶ms_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> --- .../net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)