wifi: cfg80211: wext: add extra SIOCSIWSCAN data check

[PATCH AUTOSEL 4.19] wifi: cfg80211: wext: add extra SIOCSIWSCAN data check

Posted by Sasha Levin 1 year, 5 months ago

From: Dmitry Antipov <dmantipov@yandex.ru>

[ Upstream commit 6ef09cdc5ba0f93826c09d810c141a8d103a80fc ]

In 'cfg80211_wext_siwscan()', add extra check whether number of
channels passed via 'ioctl(sock, SIOCSIWSCAN, ...)' doesn't exceed
IW_MAX_FREQUENCIES and reject invalid request with -EINVAL otherwise.

Reported-by: syzbot+253cd2d2491df77c93ac@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=253cd2d2491df77c93ac
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
Link: https://msgid.link/20240531032010.451295-1-dmantipov@yandex.ru
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 net/wireless/scan.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index dacb9ceee3efd..0dc27703443c8 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1405,10 +1405,14 @@ int cfg80211_wext_siwscan(struct net_device *dev,
 	wiphy = &rdev->wiphy;
 
 	/* Determine number of channels, needed to allocate creq */
-	if (wreq && wreq->num_channels)
+	if (wreq && wreq->num_channels) {
+		/* Passed from userspace so should be checked */
+		if (unlikely(wreq->num_channels > IW_MAX_FREQUENCIES))
+			return -EINVAL;
 		n_channels = wreq->num_channels;
-	else
+	} else {
 		n_channels = ieee80211_get_num_supported_channels(wiphy);
+	}
 
 	creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) +
 		       n_channels * sizeof(void *),
-- 
2.43.0

Re: [PATCH AUTOSEL 4.19] wifi: cfg80211: wext: add extra SIOCSIWSCAN data check

Posted by Pavel Machek 1 year, 5 months ago

Hi!

> [ Upstream commit 6ef09cdc5ba0f93826c09d810c141a8d103a80fc ]
> 
> In 'cfg80211_wext_siwscan()', add extra check whether number of
> channels passed via 'ioctl(sock, SIOCSIWSCAN, ...)' doesn't exceed
> IW_MAX_FREQUENCIES and reject invalid request with -EINVAL otherwise.

This results in very confusing code in 4.19 at least. It should goto
out for consistency, exploting kfree(NULL) to be nop. Ok, not sure we
care...

Best regards,
								Pavel
								
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index dacb9ceee3efd..0dc27703443c8 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -1405,10 +1405,14 @@ int cfg80211_wext_siwscan(struct net_device *dev,
>  	wiphy = &rdev->wiphy;
>  
>  	/* Determine number of channels, needed to allocate creq */
> -	if (wreq && wreq->num_channels)
> +	if (wreq && wreq->num_channels) {
> +		/* Passed from userspace so should be checked */
> +		if (unlikely(wreq->num_channels > IW_MAX_FREQUENCIES))
> +			return -EINVAL;
>  		n_channels = wreq->num_channels;
> -	else
> +	} else {
>  		n_channels = ieee80211_get_num_supported_channels(wiphy);
> +	}
>  
>  	creq = kzalloc(sizeof(*creq) + sizeof(struct cfg80211_ssid) +
>  		       n_channels * sizeof(void *),

-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH AUTOSEL 4.19] wifi: cfg80211: wext: add extra SIOCSIWSCAN data check

Posted by Greg KH 1 year, 5 months ago

On Wed, Jul 10, 2024 at 12:04:15PM +0200, Pavel Machek wrote:
> Hi!
> 
> > [ Upstream commit 6ef09cdc5ba0f93826c09d810c141a8d103a80fc ]
> > 
> > In 'cfg80211_wext_siwscan()', add extra check whether number of
> > channels passed via 'ioctl(sock, SIOCSIWSCAN, ...)' doesn't exceed
> > IW_MAX_FREQUENCIES and reject invalid request with -EINVAL otherwise.
> 
> This results in very confusing code in 4.19 at least. It should goto
> out for consistency, exploting kfree(NULL) to be nop. Ok, not sure we
> care...

kfree(NULL) is always supposed to be a nop, we have relied on that for
decades, that's not an issue anywhere.

greg k-h

Re: [PATCH AUTOSEL 4.19] wifi: cfg80211: wext: add extra SIOCSIWSCAN data check

Posted by Pavel Machek 1 year, 5 months ago

On Wed 2024-07-10 12:24:55, Greg KH wrote:
> On Wed, Jul 10, 2024 at 12:04:15PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > [ Upstream commit 6ef09cdc5ba0f93826c09d810c141a8d103a80fc ]
> > > 
> > > In 'cfg80211_wext_siwscan()', add extra check whether number of
> > > channels passed via 'ioctl(sock, SIOCSIWSCAN, ...)' doesn't exceed
> > > IW_MAX_FREQUENCIES and reject invalid request with -EINVAL otherwise.
> > 
> > This results in very confusing code in 4.19 at least. It should goto
> > out for consistency, exploting kfree(NULL) to be nop. Ok, not sure we
> > care...
> 
> kfree(NULL) is always supposed to be a nop, we have relied on that for
> decades, that's not an issue anywhere.

Take a look at the code, especially after this patch is applied.

BR,
								Pavel
-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany