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
