| Message ID | 018ecf13-e371-4b39-8946-c7510baf916b@stanley.mountain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] xfrm: fix integer overflow in xfrm_replay_state_esn_len() | expand |
On Tue, Jan 21, 2025 at 02:16:01PM +0300, Dan Carpenter wrote: > The problem is that "replay_esn->bmp_len" comes from the user and it's > a u32. The xfrm_replay_state_esn_len() function also returns a u32. > So if we choose a ->bmp_len which very high then the total will be > more than UINT_MAX and value will be truncated when we return. The > returned value will be smaller than expected causing problems in the > caller. > > To fix this: > 1) Use size_add() and size_mul(). This change is necessary for 32bit > systems. > 2) Change the type of xfrm_replay_state_esn_len() and related variables > from u32 to size_t. > 3) Remove the casts to (int). The size should never be negative. > Generally, values which come from size_add/mul() should stay as type > size_t and not be truncated to user fewer than all the bytes in a > unsigned long. > > Cc: stable@vger.kernel.org > Fixes: 9736acf395d3 ("xfrm: Add basic infrastructure to support IPsec extended sequence numbers") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > The one caller that I didn't modify was xfrm_sa_len(). That's a bit > complicated and also I'm kind of hoping that we don't handle user > controlled data in that function? The place where we definitely are > handling user data is in xfrm_alloc_replay_state_esn() and this patch > fixes that. Yes, that is a bit "complex". FWIIW, my opinion is that your patch is correct and it improves things - even if the end result may still have imperfections. And for that reason I'm in favour of it being accepted. Reviewed-by: Simon Horman <horms@kernel.org>
On Wed, Jan 22, 2025 at 12:39:36PM +0000, Simon Horman wrote: > > The one caller that I didn't modify was xfrm_sa_len(). That's a bit > > complicated and also I'm kind of hoping that we don't handle user > > controlled data in that function? The place where we definitely are > > handling user data is in xfrm_alloc_replay_state_esn() and this patch > > fixes that. > > Yes, that is a bit "complex". > I don't have a reason to suspect xfrm_sa_len() but if we were to write a paranoid version of it then I've written that draft below. I stole Herbert's xfrm_kblen2klen() function[1]. Also the nlmsg_new() function would need to be updated as well. https://lore.kernel.org/all/Z2KZC71JZ0QnrhfU@gondor.apana.org.au/ regards, dan carpenter diff --git a/include/net/netlink.h b/include/net/netlink.h index e015ffbed819..ca7a8152e6d4 100644 --- a/include/net/netlink.h +++ b/include/net/netlink.h @@ -1015,6 +1015,8 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb, */ static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags) { + if (payload > INT_MAX) + return NULL; return alloc_skb(nlmsg_total_size(payload), flags); } diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 08c6d6f0179f..ea51a730f268 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -3575,61 +3575,69 @@ static int xfrm_notify_sa_flush(const struct km_event *c) return xfrm_nlmsg_multicast(net, skb, 0, XFRMNLGRP_SA); } +static inline unsigned int xfrm_kblen2klen(unsigned int klen_in_bits) +{ + return klen_in_bits / 8 + !!(klen_in_bits & 7); +} -static inline unsigned int xfrm_sa_len(struct xfrm_state *x) +static inline size_t xfrm_sa_len(struct xfrm_state *x) { - unsigned int l = 0; + size_t l = 0; + if (x->aead) - l += nla_total_size(aead_len(x->aead)); + l = size_add(l, nla_total_size(aead_len(x->aead))); if (x->aalg) { - l += nla_total_size(sizeof(struct xfrm_algo) + - (x->aalg->alg_key_len + 7) / 8); - l += nla_total_size(xfrm_alg_auth_len(x->aalg)); + unsigned int old_size; + + old_size = nla_total_size(sizeof(struct xfrm_algo) + + xfrm_kblen2klen(x->aalg->alg_key_len)); + l = size_add(l, old_size); + l = size_add(l, nla_total_size(xfrm_alg_auth_len(x->aalg))); } if (x->ealg) - l += nla_total_size(xfrm_alg_len(x->ealg)); + l = size_add(l, nla_total_size(xfrm_alg_len(x->ealg))); if (x->calg) - l += nla_total_size(sizeof(*x->calg)); + l = size_add(l, nla_total_size(sizeof(*x->calg))); if (x->encap) - l += nla_total_size(sizeof(*x->encap)); + l = size_add(l, nla_total_size(sizeof(*x->encap))); if (x->tfcpad) - l += nla_total_size(sizeof(x->tfcpad)); + l = size_add(l, nla_total_size(sizeof(x->tfcpad))); if (x->replay_esn) - l += nla_total_size(xfrm_replay_state_esn_len(x->replay_esn)); + l = size_add(l, nla_total_size(xfrm_replay_state_esn_len(x->replay_esn))); else - l += nla_total_size(sizeof(struct xfrm_replay_state)); + l = size_add(l, nla_total_size(sizeof(struct xfrm_replay_state))); if (x->security) - l += nla_total_size(sizeof(struct xfrm_user_sec_ctx) + - x->security->ctx_len); + l = size_add(l, nla_total_size(sizeof(struct xfrm_user_sec_ctx) + + x->security->ctx_len)); if (x->coaddr) - l += nla_total_size(sizeof(*x->coaddr)); + l = size_add(l, nla_total_size(sizeof(*x->coaddr))); if (x->props.extra_flags) - l += nla_total_size(sizeof(x->props.extra_flags)); + l = size_add(l, nla_total_size(sizeof(x->props.extra_flags))); if (x->xso.dev) - l += nla_total_size(sizeof(struct xfrm_user_offload)); + l = size_add(l, nla_total_size(sizeof(struct xfrm_user_offload))); if (x->props.smark.v | x->props.smark.m) { - l += nla_total_size(sizeof(x->props.smark.v)); - l += nla_total_size(sizeof(x->props.smark.m)); + l = size_add(l, nla_total_size(sizeof(x->props.smark.v))); + l = size_add(l, nla_total_size(sizeof(x->props.smark.m))); } if (x->if_id) - l += nla_total_size(sizeof(x->if_id)); + l = size_add(l, nla_total_size(sizeof(x->if_id))); if (x->pcpu_num) - l += nla_total_size(sizeof(x->pcpu_num)); + l = size_add(l, nla_total_size(sizeof(x->pcpu_num))); /* Must count x->lastused as it may become non-zero behind our back. */ - l += nla_total_size_64bit(sizeof(u64)); + l = size_add(l, nla_total_size_64bit(sizeof(u64))); if (x->mapping_maxage) - l += nla_total_size(sizeof(x->mapping_maxage)); + l = size_add(l, nla_total_size(sizeof(x->mapping_maxage))); if (x->dir) - l += nla_total_size(sizeof(x->dir)); + l = size_add(l, nla_total_size(sizeof(x->dir))); if (x->nat_keepalive_interval) - l += nla_total_size(sizeof(x->nat_keepalive_interval)); + l = size_add(l, nla_total_size(sizeof(x->nat_keepalive_interval))); if (x->mode_cbs && x->mode_cbs->sa_len) - l += x->mode_cbs->sa_len(x); + l = size_add(l, x->mode_cbs->sa_len(x)); return l; } @@ -3641,17 +3649,17 @@ static int xfrm_notify_sa(struct xfrm_state *x, const struct km_event *c) struct xfrm_usersa_id *id; struct nlmsghdr *nlh; struct sk_buff *skb; - unsigned int len = xfrm_sa_len(x); + size_t len = xfrm_sa_len(x); unsigned int headlen; int err; headlen = sizeof(*p); if (c->event == XFRM_MSG_DELSA) { - len += nla_total_size(headlen); + len = size_add(len, nla_total_size(headlen)); headlen = sizeof(*id); - len += nla_total_size(sizeof(struct xfrm_mark)); + len = size_add(len, nla_total_size(sizeof(struct xfrm_mark))); } - len += NLMSG_ALIGN(headlen); + len = size_add(len, NLMSG_ALIGN(headlen)); skb = nlmsg_new(len, GFP_ATOMIC); if (skb == NULL)
On Wed, Jan 22, 2025 at 04:16:48PM +0300, Dan Carpenter wrote: > On Wed, Jan 22, 2025 at 12:39:36PM +0000, Simon Horman wrote: > > > The one caller that I didn't modify was xfrm_sa_len(). That's a bit > > > complicated and also I'm kind of hoping that we don't handle user > > > controlled data in that function? The place where we definitely are > > > handling user data is in xfrm_alloc_replay_state_esn() and this patch > > > fixes that. > > > > Yes, that is a bit "complex". > > > > I don't have a reason to suspect xfrm_sa_len() but if we were to write > a paranoid version of it then I've written that draft below. I stole > Herbert's xfrm_kblen2klen() function[1]. Also the nlmsg_new() function > would need to be updated as well. > > https://lore.kernel.org/all/Z2KZC71JZ0QnrhfU@gondor.apana.org.au/ > > regards, > dan carpenter > > diff --git a/include/net/netlink.h b/include/net/netlink.h > index e015ffbed819..ca7a8152e6d4 100644 > --- a/include/net/netlink.h > +++ b/include/net/netlink.h > @@ -1015,6 +1015,8 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb, > */ > static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags) > { > + if (payload > INT_MAX) > + return NULL; > return alloc_skb(nlmsg_total_size(payload), flags); > } Actually, this chunk is necessary. Let me sent that by itself. regards, dan carpenter
Hi Dan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on net/main]
url: https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/xfrm-fix-integer-overflow-in-xfrm_replay_state_esn_len/20250121-191827
base: net/main
patch link: https://lore.kernel.org/r/018ecf13-e371-4b39-8946-c7510baf916b%40stanley.mountain
patch subject: [PATCH net] xfrm: fix integer overflow in xfrm_replay_state_esn_len()
config: mips-allyesconfig (https://download.01.org/0day-ci/archive/20250123/202501230035.cFbLTHtZ-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250123/202501230035.cFbLTHtZ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501230035.cFbLTHtZ-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from include/linux/string.h:389,
from include/linux/bitmap.h:13,
from include/linux/cpumask.h:12,
from arch/mips/include/asm/processor.h:15,
from arch/mips/include/asm/thread_info.h:16,
from include/linux/thread_info.h:60,
from include/asm-generic/preempt.h:5,
from ./arch/mips/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:79,
from include/linux/spinlock.h:56,
from include/net/xfrm.h:7,
from net/xfrm/xfrm_replay.c:10:
In function 'memcmp',
inlined from 'xfrm_replay_notify_bmp' at net/xfrm/xfrm_replay.c:336:7:
>> include/linux/fortify-string.h:120:33: warning: '__builtin_memcmp' specified bound 4294967295 exceeds maximum object size 2147483647 [-Wstringop-overread]
120 | #define __underlying_memcmp __builtin_memcmp
| ^
include/linux/fortify-string.h:727:16: note: in expansion of macro '__underlying_memcmp'
727 | return __underlying_memcmp(p, q, size);
| ^~~~~~~~~~~~~~~~~~~
net/xfrm/xfrm_replay.c: In function 'xfrm_replay_notify_bmp':
net/xfrm/xfrm_replay.c:308:53: note: source object allocated here
308 | struct xfrm_replay_state_esn *replay_esn = x->replay_esn;
| ~^~~~~~~~~~~~
In function 'memcmp',
inlined from 'xfrm_replay_notify_esn' at net/xfrm/xfrm_replay.c:402:7:
>> include/linux/fortify-string.h:120:33: warning: '__builtin_memcmp' specified bound 4294967295 exceeds maximum object size 2147483647 [-Wstringop-overread]
120 | #define __underlying_memcmp __builtin_memcmp
| ^
include/linux/fortify-string.h:727:16: note: in expansion of macro '__underlying_memcmp'
727 | return __underlying_memcmp(p, q, size);
| ^~~~~~~~~~~~~~~~~~~
net/xfrm/xfrm_replay.c: In function 'xfrm_replay_notify_esn':
net/xfrm/xfrm_replay.c:360:53: note: source object allocated here
360 | struct xfrm_replay_state_esn *replay_esn = x->replay_esn;
| ~^~~~~~~~~~~~
vim +/__builtin_memcmp +120 include/linux/fortify-string.h
78a498c3a227f2 Alexander Potapenko 2022-10-24 118
78a498c3a227f2 Alexander Potapenko 2022-10-24 119 #define __underlying_memchr __builtin_memchr
78a498c3a227f2 Alexander Potapenko 2022-10-24 @120 #define __underlying_memcmp __builtin_memcmp
a28a6e860c6cf2 Francis Laniel 2021-02-25 121 #define __underlying_strcat __builtin_strcat
a28a6e860c6cf2 Francis Laniel 2021-02-25 122 #define __underlying_strcpy __builtin_strcpy
a28a6e860c6cf2 Francis Laniel 2021-02-25 123 #define __underlying_strlen __builtin_strlen
a28a6e860c6cf2 Francis Laniel 2021-02-25 124 #define __underlying_strncat __builtin_strncat
a28a6e860c6cf2 Francis Laniel 2021-02-25 125 #define __underlying_strncpy __builtin_strncpy
2e577732e8d28b Andrey Konovalov 2024-05-17 126
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index ed4b83696c77..0a42614d7840 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1981,9 +1981,9 @@ static inline unsigned int xfrm_alg_auth_len(const struct xfrm_algo_auth *alg) return sizeof(*alg) + ((alg->alg_key_len + 7) / 8); } -static inline unsigned int xfrm_replay_state_esn_len(struct xfrm_replay_state_esn *replay_esn) +static inline size_t xfrm_replay_state_esn_len(struct xfrm_replay_state_esn *replay_esn) { - return sizeof(*replay_esn) + replay_esn->bmp_len * sizeof(__u32); + return size_add(sizeof(*replay_esn), size_mul(replay_esn->bmp_len, sizeof(__u32))); } #ifdef CONFIG_XFRM_MIGRATE diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 08c6d6f0179f..4bfa72547dab 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -151,7 +151,7 @@ static inline int verify_replay(struct xfrm_usersa_info *p, return -EINVAL; } - if (nla_len(rt) < (int)xfrm_replay_state_esn_len(rs) && + if (nla_len(rt) < xfrm_replay_state_esn_len(rs) && nla_len(rt) != sizeof(*rs)) { NL_SET_ERR_MSG(extack, "ESN attribute is too short to fit the full bitmap length"); return -EINVAL; @@ -681,7 +681,7 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es struct netlink_ext_ack *extack) { struct xfrm_replay_state_esn *up; - unsigned int ulen; + size_t ulen; if (!replay_esn || !rp) return 0; @@ -691,7 +691,7 @@ static inline int xfrm_replay_verify_len(struct xfrm_replay_state_esn *replay_es /* Check the overall length and the internal bitmap length to avoid * potential overflow. */ - if (nla_len(rp) < (int)ulen) { + if (nla_len(rp) < ulen) { NL_SET_ERR_MSG(extack, "ESN attribute is too short"); return -EINVAL; } @@ -719,14 +719,14 @@ static int xfrm_alloc_replay_state_esn(struct xfrm_replay_state_esn **replay_esn struct nlattr *rta) { struct xfrm_replay_state_esn *p, *pp, *up; - unsigned int klen, ulen; + size_t klen, ulen; if (!rta) return 0; up = nla_data(rta); klen = xfrm_replay_state_esn_len(up); - ulen = nla_len(rta) >= (int)klen ? klen : sizeof(*up); + ulen = nla_len(rta) >= klen ? klen : sizeof(*up); p = kzalloc(klen, GFP_KERNEL); if (!p)
The problem is that "replay_esn->bmp_len" comes from the user and it's a u32. The xfrm_replay_state_esn_len() function also returns a u32. So if we choose a ->bmp_len which very high then the total will be more than UINT_MAX and value will be truncated when we return. The returned value will be smaller than expected causing problems in the caller. To fix this: 1) Use size_add() and size_mul(). This change is necessary for 32bit systems. 2) Change the type of xfrm_replay_state_esn_len() and related variables from u32 to size_t. 3) Remove the casts to (int). The size should never be negative. Generally, values which come from size_add/mul() should stay as type size_t and not be truncated to user fewer than all the bytes in a unsigned long. Cc: stable@vger.kernel.org Fixes: 9736acf395d3 ("xfrm: Add basic infrastructure to support IPsec extended sequence numbers") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- The one caller that I didn't modify was xfrm_sa_len(). That's a bit complicated and also I'm kind of hoping that we don't handle user controlled data in that function? The place where we definitely are handling user data is in xfrm_alloc_replay_state_esn() and this patch fixes that. include/net/xfrm.h | 4 ++-- net/xfrm/xfrm_user.c | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-)