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

From: Johannes Berg <johannes@sipsolutions.net>
To: Kees Cook <keescook@chromium.org>,
	Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: 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 19:40:26 +0100	[thread overview]
Message-ID: <01e3663e9e1418a183ee86251e0352256494ee28.camel@sipsolutions.net> (raw)
In-Reply-To: <202311301016.84D0010@keescook>

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.)

> Does adding an upper bounds sanity check help as a work-around, like:

So ... no idea!

I guess I can push something to a branch and see if the robot picks it
up ...

johannes