| Message ID | 20241007222301.24154-2-alpernebiyasak@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2,1/2] wifi: mwifiex: Fix memcpy() field-spanning write warning in mwifiex_config_scan() | expand |
Hi Alper, On Tue, Oct 08, 2024 at 01:20:55AM +0300, Alper Nebi Yasak wrote: > Add the __counted_by compiler attribute to the flexible array member > `ssid` to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and > CONFIG_FORTIFY_SOURCE. > > Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> > --- > I've mimicked the commit messages from `git log -S"__counted_by("`. > Since they refer to UBSAN I tried testing with CONFIG_KASAN (w/ > kasan_multi_shot), CONFIG_UBSAN and CONFIG_FORTIFY_SOURCE. I do not > get any errors relating to this module with those options, but have > others -- I even had to test on another board altogether. It's possible you don't have a new enough compiler to enable the improved array bounds checks? Supposedly, that's new in GCC 15 and Clang 18. But I'm not too familiar. > This attribute was suggested in reviews, I don't fully understand it, > but I am not sure it is correct in the context of this comment from > scan.c (with irrelelvant parts omitted): > > ssid_len = user_scan_in->ssid_list[i].ssid_len; > > wildcard_ssid_tlv = > (struct mwifiex_ie_types_wildcard_ssid_params *) > tlv_pos; > > /* > * max_ssid_length = 0 tells firmware to perform > * specific scan for the SSID filled, whereas > * max_ssid_length = IEEE80211_MAX_SSID_LEN is for > * wildcard scan. > */ > if (ssid_len) > wildcard_ssid_tlv->max_ssid_length = 0; > else > wildcard_ssid_tlv->max_ssid_length = > IEEE80211_MAX_SSID_LEN; > > memcpy(wildcard_ssid_tlv->ssid, > user_scan_in->ssid_list[i].ssid, ssid_len); > > I expect we want to use __counted_by(ssid_len) instead, but do not have > it in the struct. And max_ssid_length = 0 when ssid[] is non-empty, so > is it really appropriate as the __counted_by()? But then again, I > couldn't get this to produce a warning. The correct length would be derived from header.len. But IIUC, __counted_by neither supports nested structs, nor derived values. But anyway, like you suspect, __counted_by(max_ssid_length) is actually wrong. I'd be happy to be enlighted by other experts on CC, but otherwise, that's a "NACK" for patch 2. Thanks for looking into this though! Regards, Brian > Changes in v2: > - Add patch to annotate ssid field with __counted_by(max_ssid_length) > > drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 4a96281792cc..e4e35ba35454 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -875,7 +875,7 @@ struct mwifiex_ietypes_chanstats { > struct mwifiex_ie_types_wildcard_ssid_params { > struct mwifiex_ie_types_header header; > u8 max_ssid_length; > - u8 ssid[]; > + u8 ssid[] __counted_by(max_ssid_length); > } __packed; > > #define TSF_DATA_SIZE 8 > -- > 2.45.2 >
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 4a96281792cc..e4e35ba35454 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -875,7 +875,7 @@ struct mwifiex_ietypes_chanstats { struct mwifiex_ie_types_wildcard_ssid_params { struct mwifiex_ie_types_header header; u8 max_ssid_length; - u8 ssid[]; + u8 ssid[] __counted_by(max_ssid_length); } __packed; #define TSF_DATA_SIZE 8
Add the __counted_by compiler attribute to the flexible array member `ssid` to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and CONFIG_FORTIFY_SOURCE. Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com> --- I've mimicked the commit messages from `git log -S"__counted_by("`. Since they refer to UBSAN I tried testing with CONFIG_KASAN (w/ kasan_multi_shot), CONFIG_UBSAN and CONFIG_FORTIFY_SOURCE. I do not get any errors relating to this module with those options, but have others -- I even had to test on another board altogether. This attribute was suggested in reviews, I don't fully understand it, but I am not sure it is correct in the context of this comment from scan.c (with irrelelvant parts omitted): ssid_len = user_scan_in->ssid_list[i].ssid_len; wildcard_ssid_tlv = (struct mwifiex_ie_types_wildcard_ssid_params *) tlv_pos; /* * max_ssid_length = 0 tells firmware to perform * specific scan for the SSID filled, whereas * max_ssid_length = IEEE80211_MAX_SSID_LEN is for * wildcard scan. */ if (ssid_len) wildcard_ssid_tlv->max_ssid_length = 0; else wildcard_ssid_tlv->max_ssid_length = IEEE80211_MAX_SSID_LEN; memcpy(wildcard_ssid_tlv->ssid, user_scan_in->ssid_list[i].ssid, ssid_len); I expect we want to use __counted_by(ssid_len) instead, but do not have it in the struct. And max_ssid_length = 0 when ssid[] is non-empty, so is it really appropriate as the __counted_by()? But then again, I couldn't get this to produce a warning. Changes in v2: - Add patch to annotate ssid field with __counted_by(max_ssid_length) drivers/net/wireless/marvell/mwifiex/fw.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)