| Message ID | 20240716174011.236849-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: wireless: fix more UBSAN noise in cfg80211_conn_scan() | expand |
On Tue, Jul 16, 2024 at 08:40:11PM +0300, Dmitry Antipov wrote: > Looking at https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774 > and trying to reproduce it with CONFIG_UBSAN enabled, I've noticed the > following: > > UBSAN: array-index-out-of-bounds in net/wireless/sme.c:95:3 > index 0 is out of range for type 'struct ieee80211_channel *[]' > CPU: 3 PID: 4993 Comm: repro Not tainted 6.10.0-01155-gd67978318827 #5 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <...> > Call Trace: > <TASK> > dump_stack_lvl+0x1c2/0x2a0 > ? __pfx_dump_stack_lvl+0x10/0x10 > ? __pfx__printk+0x10/0x10 > ? __local_bh_enable_ip+0x12e/0x1c0 > __ubsan_handle_out_of_bounds+0x127/0x150 > cfg80211_conn_scan+0xd8e/0xf30 > cfg80211_connect+0x1400/0x1c30 > nl80211_connect+0x1549/0x1a70 > ...<the rest is not too useful...> > > This is very similar to 92ecbb3ac6f3 ("wifi: mac80211: fix UBSAN noise > in ieee80211_prep_hw_scan()"), so just fix it in the same way by setting > 'request->n_channels' early to help '__counted_by()' work as expected. > And the same 'kmalloc()' math adjustment is also applicable. > > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> Nice catch! Yes, this looks correct. Reviewed-by: Kees Cook <kees@kernel.org>
Hmm. On Tue, 2024-07-16 at 20:40 +0300, Dmitry Antipov wrote: > diff --git a/net/wireless/sme.c b/net/wireless/sme.c > index a8ad55f11133..f5da45331847 100644 > --- a/net/wireless/sme.c > +++ b/net/wireless/sme.c > @@ -77,12 +77,16 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev) > else > n_channels = ieee80211_get_num_supported_channels(wdev->wiphy); > > - request = kzalloc(sizeof(*request) + sizeof(request->ssids[0]) + > - sizeof(request->channels[0]) * n_channels, > - GFP_KERNEL); > + request = kzalloc(struct_size(request, channels, n_channels) + > + sizeof(request->ssids[0]), GFP_KERNEL); That makes sense, sure. > if (!request) > return -ENOMEM; > > + /* None of the channels are actually set > + * up but let UBSAN know the boundaries. > + */ > + request->n_channels = n_channels; Also makes sense, so we tell it how many we allocated early. Note netdev we dropped the special comment style requirement, so wouldn't mind /* * None of ... * ... */ here either. > + > if (wdev->conn->params.channel) { > enum nl80211_band band = wdev->conn->params.channel->band; > struct ieee80211_supported_band *sband = > @@ -112,9 +116,9 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev) > } > request->rates[band] = (1 << bands->n_bitrates) - 1; > } > - n_channels = i; > + request->n_channels = i; So this tells it how many were actually used, in this branch, makes sense. Functionally, all of this seems OK. However, > request->ssids = (void *)&request->channels[n_channels]; is this not checked? I mean, if you have n_channels=5 and then take &channels[5] I can see how that makes sense, but arguably the compiler might complain if you have &channels[10] for an array you told it has 5 entries? johannes
diff --git a/net/wireless/sme.c b/net/wireless/sme.c index a8ad55f11133..f5da45331847 100644 --- a/net/wireless/sme.c +++ b/net/wireless/sme.c @@ -77,12 +77,16 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev) else n_channels = ieee80211_get_num_supported_channels(wdev->wiphy); - request = kzalloc(sizeof(*request) + sizeof(request->ssids[0]) + - sizeof(request->channels[0]) * n_channels, - GFP_KERNEL); + request = kzalloc(struct_size(request, channels, n_channels) + + sizeof(request->ssids[0]), GFP_KERNEL); if (!request) return -ENOMEM; + /* None of the channels are actually set + * up but let UBSAN know the boundaries. + */ + request->n_channels = n_channels; + if (wdev->conn->params.channel) { enum nl80211_band band = wdev->conn->params.channel->band; struct ieee80211_supported_band *sband = @@ -112,9 +116,9 @@ static int cfg80211_conn_scan(struct wireless_dev *wdev) } request->rates[band] = (1 << bands->n_bitrates) - 1; } - n_channels = i; + request->n_channels = i; } - request->n_channels = n_channels; + request->ssids = (void *)&request->channels[n_channels]; request->n_ssids = 1;
Looking at https://syzkaller.appspot.com/bug?extid=d5dc2801166df6d34774 and trying to reproduce it with CONFIG_UBSAN enabled, I've noticed the following: UBSAN: array-index-out-of-bounds in net/wireless/sme.c:95:3 index 0 is out of range for type 'struct ieee80211_channel *[]' CPU: 3 PID: 4993 Comm: repro Not tainted 6.10.0-01155-gd67978318827 #5 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS <...> Call Trace: <TASK> dump_stack_lvl+0x1c2/0x2a0 ? __pfx_dump_stack_lvl+0x10/0x10 ? __pfx__printk+0x10/0x10 ? __local_bh_enable_ip+0x12e/0x1c0 __ubsan_handle_out_of_bounds+0x127/0x150 cfg80211_conn_scan+0xd8e/0xf30 cfg80211_connect+0x1400/0x1c30 nl80211_connect+0x1549/0x1a70 ...<the rest is not too useful...> This is very similar to 92ecbb3ac6f3 ("wifi: mac80211: fix UBSAN noise in ieee80211_prep_hw_scan()"), so just fix it in the same way by setting 'request->n_channels' early to help '__counted_by()' work as expected. And the same 'kmalloc()' math adjustment is also applicable. Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- net/wireless/sme.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)