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

[Kees Cook <keescook@chromium.org>]

On Tue, Nov 28, 2023 at 01:01:20PM -0800, Jeff Johnson wrote:
> On 11/28/2023 10:23 AM, Johannes Berg wrote:
> > On Tue, 2023-11-28 at 15:44 +0100, Michael Walle wrote:
> >> Hi,
> >>
> >>> net/wireless/nl80211.c: In function 'nl80211_set_cqm_rssi.isra':
> >>> net/wireless/nl80211.c:12892:17: warning: 'memcpy' specified bound
> >>> 18446744073709551615 exceeds maximum object size 9223372036854775807
> >>> [-Wstringop-overflow=]
> >>
> >> FWIW, I'm getting the same error with the current next (next-20231128).
> >>
> > 
> > I actually forgot about that, but does anyone actually know what this is
> > trying to tell me?
> > 
> > The code seems to be
> > 
> >         if (n_thresholds) {
> >                 cqm_config = kzalloc(struct_size(cqm_config, rssi_thresholds,
> >                                                  n_thresholds),
> >                                      GFP_KERNEL);
> >                 if (!cqm_config)
> >                         return -ENOMEM;
> > 
> >                 cqm_config->rssi_hyst = hysteresis;
> >                 cqm_config->n_rssi_thresholds = n_thresholds;
> >                 memcpy(cqm_config->rssi_thresholds, thresholds,
> >                        flex_array_size(cqm_config, rssi_thresholds,
> >                                        n_thresholds));
> > 
> > 
> > Or does it just want to say n_thresholds shouldn't be a signed variable?
> 
> +Kees for flex array education :)

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

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

Does this warning go away if "len" is made unsigned?

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

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index d0f499227c29..2cb78ac44b6c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12855,6 +12855,9 @@ static int nl80211_set_cqm_rssi(struct genl_info *info,
 	s32 prev = S32_MIN;
 	int i, err;
 
+	if (n_thresholds > INT_MAX / sizeof(*thresholds))
+		return -EINVAL;
+
 	/* Check all values negative and sorted */
 	for (i = 0; i < n_thresholds; i++) {
 		if (thresholds[i] > 0 || thresholds[i] <= prev)



-- 
Kees Cook