On Thu, 2023-11-30 at 10:55 -0800, Kees Cook wrote:
> 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?
>
> Actually, this alone fixes it too:
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 167b91348e57..c59679524705 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -1214,9 +1214,9 @@ static inline void *nla_data(const struct nlattr *nla)
> * nla_len - length of payload
> * @nla: netlink attribute
> */
> -static inline int nla_len(const struct nlattr *nla)
> +static inline u16 nla_len(const struct nlattr *nla)
> {
> - return nla->nla_len - NLA_HDRLEN;
> + return nla->nla_len > NLA_HDRLEN ? nla->nla_len - NLA_HDRLEN : 0;
> }
>
Heh. If you can sell that to Jakub I don't mind, but that might be a
harder sell than the int/u32 in our code...
johannes
