| Message ID | 20240905150400.126386-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: cfg80211: fix UBSAN noise in cfg80211_wext_siwscan() | expand |
On Thu, Sep 05, 2024 at 06:04:00PM +0300, Dmitry Antipov wrote: > Looking at https://syzkaller.appspot.com/bug?extid=1a3986bbd3169c307819 > and running reproducer with CONFIG_UBSAN_BOUNDS, I've noticed the > following: > > [ T4985] UBSAN: array-index-out-of-bounds in net/wireless/scan.c:3479:25 > [ T4985] index 164 is out of range for type 'struct ieee80211_channel *[]' > <...skipped...> > [ T4985] Call Trace: > [ T4985] <TASK> > [ T4985] dump_stack_lvl+0x1c2/0x2a0 > [ T4985] ? __pfx_dump_stack_lvl+0x10/0x10 > [ T4985] ? __pfx__printk+0x10/0x10 > [ T4985] __ubsan_handle_out_of_bounds+0x127/0x150 > [ T4985] cfg80211_wext_siwscan+0x11a4/0x1260 > <...the rest is not too useful...> > > Even if we do 'creq->n_channels = n_channels' before 'creq->ssids = > (void *)&creq->channels[n_channels]', UBSAN treats the latter as > off-by-one error. Fix this by using pointer arithmetic rather than > an expression with explicit array indexing and use convenient > 'struct_size()' to simplify the math here and in 'kzalloc()' above. > > Fixes: 5ba63533bbf6 ("cfg80211: fix alignment problem in scan request") > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> This looks correct -- the offset is based on the allocation base, not the array within the struct, so no array-out-of-bounds warning will happen. Reviewed-by: Kees Cook <kees@kernel.org>
diff --git a/net/wireless/scan.c b/net/wireless/scan.c index 64eeed82d43d..d747d5f63278 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -3467,8 +3467,8 @@ int cfg80211_wext_siwscan(struct net_device *dev, n_channels = ieee80211_get_num_supported_channels(wiphy); } - creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) + - n_channels * sizeof(void *), + creq = kzalloc(struct_size(creq, channels, n_channels) + + sizeof(struct cfg80211_ssid), GFP_ATOMIC); if (!creq) return -ENOMEM; @@ -3476,7 +3476,7 @@ int cfg80211_wext_siwscan(struct net_device *dev, creq->wiphy = wiphy; creq->wdev = dev->ieee80211_ptr; /* SSIDs come after channels */ - creq->ssids = (void *)&creq->channels[n_channels]; + creq->ssids = (void *)creq + struct_size(creq, channels, n_channels); creq->n_channels = n_channels; creq->n_ssids = 1; creq->scan_start = jiffies;
Looking at https://syzkaller.appspot.com/bug?extid=1a3986bbd3169c307819 and running reproducer with CONFIG_UBSAN_BOUNDS, I've noticed the following: [ T4985] UBSAN: array-index-out-of-bounds in net/wireless/scan.c:3479:25 [ T4985] index 164 is out of range for type 'struct ieee80211_channel *[]' <...skipped...> [ T4985] Call Trace: [ T4985] <TASK> [ T4985] dump_stack_lvl+0x1c2/0x2a0 [ T4985] ? __pfx_dump_stack_lvl+0x10/0x10 [ T4985] ? __pfx__printk+0x10/0x10 [ T4985] __ubsan_handle_out_of_bounds+0x127/0x150 [ T4985] cfg80211_wext_siwscan+0x11a4/0x1260 <...the rest is not too useful...> Even if we do 'creq->n_channels = n_channels' before 'creq->ssids = (void *)&creq->channels[n_channels]', UBSAN treats the latter as off-by-one error. Fix this by using pointer arithmetic rather than an expression with explicit array indexing and use convenient 'struct_size()' to simplify the math here and in 'kzalloc()' above. Fixes: 5ba63533bbf6 ("cfg80211: fix alignment problem in scan request") Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- net/wireless/scan.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)