Re: [RFC PATCH] wifi: cfg80211: fix CQM for non-range use

From: Kees Cook <keescook@chromium.org>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>,
	Michael Walle <mwalle@kernel.org>,
	lkp@intel.com, oe-kbuild-all@lists.linux.dev,
	linux-wireless@vger.kernel.org,
	Max Schulze <max.schulze@online.de>
Subject: Re: [RFC PATCH] wifi: cfg80211: fix CQM for non-range use
Date: Thu, 30 Nov 2023 10:52:33 -0800	[thread overview]
Message-ID: <202311301050.C4BFFB9837@keescook> (raw)
In-Reply-To: <01e3663e9e1418a183ee86251e0352256494ee28.camel@sipsolutions.net>

On Thu, Nov 30, 2023 at 07:40:26PM +0100, Johannes Berg wrote:
> On Thu, 2023-11-30 at 10:32 -0800, Kees Cook wrote:
> > Yeah, I would expect this to mean that there is a code path that
> > GCC found where the value could overflow. It does this when a variable
> > "value range" gets bounded (e.g. an int isn't the full -INT_MAX to INT_MAX
> > range).And flex_array_size() was designed to saturate at SIZE_MIX rather
> > than wrapping around to an unexpected small value, so these are playing
> > together it seems.
> > 
> > However, I would have expected the kzalloc() to blow up _first_.
> 
> Hmm.
> 
> > Regardless, I suspect the addition of "if (n_thresholds > 1)" is what is
> > tripping GCC.
> > 
> >                 int len = nla_len(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
> > 		...
> >                 return nl80211_set_cqm_rssi(info, thresholds, len / 4,
> >                                             hysteresis);
> > 
> > Now it "knows" there is a path where n_threasholds could be [2,
> > INT_MAX].
> 
> Yeah, it's not _really_ bounded, apart from the message length? But then
> struct_size() should saturate and fail? But I guess it cannot know that,
> and limits the object size to 1<<63 - 1 whereas the copy is 1<<64 - 1...
> 
> > Does this warning go away if "len" is made unsigned?
> 
> Thing is, neither Kalle nor I can even reproduce the warning locally, so
> it's a bit hard to check ... not even with their config and gcc 12.2.0
> (nix, rather than debian though.)

I was able to see it with Ubuntu's GCC 12.3.0 and their config. This
fixed it for me:

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d0f499227c29..7735d178a393 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12845,7 +12845,7 @@ static int cfg80211_cqm_rssi_update(struct cfg80211_registered_device *rdev,
 }
 
 static int nl80211_set_cqm_rssi(struct genl_info *info,
-				const s32 *thresholds, int n_thresholds,
+				const s32 *thresholds, u32 n_thresholds,
 				u32 hysteresis)
 {
 	struct cfg80211_registered_device *rdev = info->user_ptr[0];
@@ -12948,7 +12948,7 @@ static int nl80211_set_cqm(struct sk_buff *skb, struct genl_info *info)
 	    attrs[NL80211_ATTR_CQM_RSSI_HYST]) {
 		const s32 *thresholds =
 			nla_data(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
-		int len = nla_len(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
+		u32 len = nla_len(attrs[NL80211_ATTR_CQM_RSSI_THOLD]);
 		u32 hysteresis = nla_get_u32(attrs[NL80211_ATTR_CQM_RSSI_HYST]);
 
 		if (len % 4)


If that's sensible, I can send a proper patch?

(Oh, it looks like nla_len is actually u16 ... should I use that instead
of u32?)

-- 
Kees Cook
