* [PATCH v2] netlink: Return unsigned value for nla_len()
@ 2023-12-02 20:25 Kees Cook
2023-12-04 9:22 ` Nicolas Dichtel
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2023-12-02 20:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Kees Cook, kernel test robot, David S. Miller, Eric Dumazet,
Paolo Abeni, Johannes Berg, Jeff Johnson, Michael Walle,
Max Schulze, netdev, linux-wireless, Nicolas Dichtel,
linux-kernel, linux-hardening
The return value from nla_len() is never expected to be negative, and can
never be more than struct nlattr::nla_len (a u16). Adjust the prototype
on the function. This will let GCC's value range optimization passes
know that the return can never be negative, and can never be larger than
u16. As recently discussed[1], this silences the following warning in
GCC 12+:
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=]
12892 | memcpy(cqm_config->rssi_thresholds, thresholds,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
12893 | flex_array_size(cqm_config, rssi_thresholds,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
12894 | n_thresholds));
| ~~~~~~~~~~~~~~
A future change would be to clamp the subtraction to make sure it never
wraps around if nla_len is somehow less than NLA_HDRLEN, which would
have the additional benefit of being defensive in the face of nlattr
corruption or logic errors.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202311090752.hWcJWAHL-lkp@intel.com/ [1]
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: Michael Walle <mwalle@kernel.org>
Cc: Max Schulze <max.schulze@online.de>
Cc: netdev@vger.kernel.org
Cc: linux-wireless@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2:
- do not clamp return value (kuba)
- adjust NLA_HDRLEN to be u16 also
v1: https://lore.kernel.org/all/20231130200058.work.520-kees@kernel.org/
---
include/net/netlink.h | 2 +-
include/uapi/linux/netlink.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 83bdf787aeee..7678a596a86b 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1200,7 +1200,7 @@ 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;
}
diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
index f87aaf28a649..270feed9fd63 100644
--- a/include/uapi/linux/netlink.h
+++ b/include/uapi/linux/netlink.h
@@ -247,7 +247,7 @@ struct nlattr {
#define NLA_ALIGNTO 4
#define NLA_ALIGN(len) (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
-#define NLA_HDRLEN ((int) NLA_ALIGN(sizeof(struct nlattr)))
+#define NLA_HDRLEN ((__u16) NLA_ALIGN(sizeof(struct nlattr)))
/* Generic 32 bitflags attribute content sent to the kernel.
*
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v2] netlink: Return unsigned value for nla_len()
2023-12-02 20:25 [PATCH v2] netlink: Return unsigned value for nla_len() Kees Cook
@ 2023-12-04 9:22 ` Nicolas Dichtel
2023-12-04 22:21 ` Kees Cook
0 siblings, 1 reply; 4+ messages in thread
From: Nicolas Dichtel @ 2023-12-04 9:22 UTC (permalink / raw)
To: Kees Cook, Jakub Kicinski
Cc: kernel test robot, David S. Miller, Eric Dumazet, Paolo Abeni,
Johannes Berg, Jeff Johnson, Michael Walle, Max Schulze, netdev,
linux-wireless, linux-kernel, linux-hardening
Le 02/12/2023 à 21:25, Kees Cook a écrit :
> The return value from nla_len() is never expected to be negative, and can
> never be more than struct nlattr::nla_len (a u16). Adjust the prototype
> on the function. This will let GCC's value range optimization passes
> know that the return can never be negative, and can never be larger than
> u16. As recently discussed[1], this silences the following warning in
> GCC 12+:
>
> 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=]
> 12892 | memcpy(cqm_config->rssi_thresholds, thresholds,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 12893 | flex_array_size(cqm_config, rssi_thresholds,
> | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 12894 | n_thresholds));
> | ~~~~~~~~~~~~~~
>
> A future change would be to clamp the subtraction to make sure it never
> wraps around if nla_len is somehow less than NLA_HDRLEN, which would
> have the additional benefit of being defensive in the face of nlattr
> corruption or logic errors.
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202311090752.hWcJWAHL-lkp@intel.com/ [1]
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Cc: Jeff Johnson <quic_jjohnson@quicinc.com>
> Cc: Michael Walle <mwalle@kernel.org>
> Cc: Max Schulze <max.schulze@online.de>
> Cc: netdev@vger.kernel.org
> Cc: linux-wireless@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v2:
> - do not clamp return value (kuba)
> - adjust NLA_HDRLEN to be u16 also
> v1: https://lore.kernel.org/all/20231130200058.work.520-kees@kernel.org/
> ---
> include/net/netlink.h | 2 +-
> include/uapi/linux/netlink.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 83bdf787aeee..7678a596a86b 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -1200,7 +1200,7 @@ 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;
> }
> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> index f87aaf28a649..270feed9fd63 100644
> --- a/include/uapi/linux/netlink.h
> +++ b/include/uapi/linux/netlink.h
> @@ -247,7 +247,7 @@ struct nlattr {
>
> #define NLA_ALIGNTO 4
> #define NLA_ALIGN(len) (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
> -#define NLA_HDRLEN ((int) NLA_ALIGN(sizeof(struct nlattr)))
> +#define NLA_HDRLEN ((__u16) NLA_ALIGN(sizeof(struct nlattr)))
I wonder if this may break the compilation of some userspace tools with errors
like comparing signed and unsigned values.
Regards,
Nicolas
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] netlink: Return unsigned value for nla_len()
2023-12-04 9:22 ` Nicolas Dichtel
@ 2023-12-04 22:21 ` Kees Cook
2023-12-05 7:58 ` Nicolas Dichtel
0 siblings, 1 reply; 4+ messages in thread
From: Kees Cook @ 2023-12-04 22:21 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: Jakub Kicinski, kernel test robot, David S. Miller, Eric Dumazet,
Paolo Abeni, Johannes Berg, Jeff Johnson, Michael Walle,
Max Schulze, netdev, linux-wireless, linux-kernel,
linux-hardening
On Mon, Dec 04, 2023 at 10:22:25AM +0100, Nicolas Dichtel wrote:
> Le 02/12/2023 à 21:25, Kees Cook a écrit :
> > The return value from nla_len() is never expected to be negative, and can
> > never be more than struct nlattr::nla_len (a u16). Adjust the prototype
> > on the function. This will let GCC's value range optimization passes
> > know that the return can never be negative, and can never be larger than
> > u16. As recently discussed[1], this silences the following warning in
> > GCC 12+:
> >
> > 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=]
> > 12892 | memcpy(cqm_config->rssi_thresholds, thresholds,
> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 12893 | flex_array_size(cqm_config, rssi_thresholds,
> > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 12894 | n_thresholds));
> > | ~~~~~~~~~~~~~~
> >
> > A future change would be to clamp the subtraction to make sure it never
> > wraps around if nla_len is somehow less than NLA_HDRLEN, which would
> > have the additional benefit of being defensive in the face of nlattr
> > corruption or logic errors.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202311090752.hWcJWAHL-lkp@intel.com/ [1]
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Paolo Abeni <pabeni@redhat.com>
> > Cc: Johannes Berg <johannes@sipsolutions.net>
> > Cc: Jeff Johnson <quic_jjohnson@quicinc.com>
> > Cc: Michael Walle <mwalle@kernel.org>
> > Cc: Max Schulze <max.schulze@online.de>
> > Cc: netdev@vger.kernel.org
> > Cc: linux-wireless@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > v2:
> > - do not clamp return value (kuba)
> > - adjust NLA_HDRLEN to be u16 also
> > v1: https://lore.kernel.org/all/20231130200058.work.520-kees@kernel.org/
> > ---
> > include/net/netlink.h | 2 +-
> > include/uapi/linux/netlink.h | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/netlink.h b/include/net/netlink.h
> > index 83bdf787aeee..7678a596a86b 100644
> > --- a/include/net/netlink.h
> > +++ b/include/net/netlink.h
> > @@ -1200,7 +1200,7 @@ 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;
> > }
> > diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
> > index f87aaf28a649..270feed9fd63 100644
> > --- a/include/uapi/linux/netlink.h
> > +++ b/include/uapi/linux/netlink.h
> > @@ -247,7 +247,7 @@ struct nlattr {
> >
> > #define NLA_ALIGNTO 4
> > #define NLA_ALIGN(len) (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
> > -#define NLA_HDRLEN ((int) NLA_ALIGN(sizeof(struct nlattr)))
> > +#define NLA_HDRLEN ((__u16) NLA_ALIGN(sizeof(struct nlattr)))
> I wonder if this may break the compilation of some userspace tools with errors
> like comparing signed and unsigned values.
Should I drop this part, then?
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] netlink: Return unsigned value for nla_len()
2023-12-04 22:21 ` Kees Cook
@ 2023-12-05 7:58 ` Nicolas Dichtel
0 siblings, 0 replies; 4+ messages in thread
From: Nicolas Dichtel @ 2023-12-05 7:58 UTC (permalink / raw)
To: Kees Cook
Cc: Jakub Kicinski, kernel test robot, David S. Miller, Eric Dumazet,
Paolo Abeni, Johannes Berg, Jeff Johnson, Michael Walle,
Max Schulze, netdev, linux-wireless, linux-kernel,
linux-hardening
Le 04/12/2023 à 23:21, Kees Cook a écrit :
[snip]
>>> diff --git a/include/uapi/linux/netlink.h b/include/uapi/linux/netlink.h
>>> index f87aaf28a649..270feed9fd63 100644
>>> --- a/include/uapi/linux/netlink.h
>>> +++ b/include/uapi/linux/netlink.h
>>> @@ -247,7 +247,7 @@ struct nlattr {
>>>
>>> #define NLA_ALIGNTO 4
>>> #define NLA_ALIGN(len) (((len) + NLA_ALIGNTO - 1) & ~(NLA_ALIGNTO - 1))
>>> -#define NLA_HDRLEN ((int) NLA_ALIGN(sizeof(struct nlattr)))
>>> +#define NLA_HDRLEN ((__u16) NLA_ALIGN(sizeof(struct nlattr)))
>> I wonder if this may break the compilation of some userspace tools with errors
>> like comparing signed and unsigned values.
>
> Should I drop this part, then?
>
Yes please.
Thank you,
Nicolas
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-05 7:58 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-02 20:25 [PATCH v2] netlink: Return unsigned value for nla_len() Kees Cook
2023-12-04 9:22 ` Nicolas Dichtel
2023-12-04 22:21 ` Kees Cook
2023-12-05 7:58 ` Nicolas Dichtel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).