lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <bbfd6f959e7ff4b567084ef3d962bf255aa25c85.camel@sipsolutions.net> Date: Thu, 30 Nov 2023 20:00:44 +0100 From: Johannes Berg <johannes@...solutions.net> To: Kees Cook <keescook@...omium.org> Cc: Jeff Johnson <quic_jjohnson@...cinc.com>, Michael Walle <mwalle@...nel.org>, lkp@...el.com, oe-kbuild-all@...ts.linux.dev, linux-wireless@...r.kernel.org, Max Schulze <max.schulze@...ine.de>, netdev@...r.kernel.org Subject: Re: [RFC PATCH] wifi: cfg80211: fix CQM for non-range use 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
Powered by blists - more mailing lists