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@xxxxxxxxxxxxxxxxxxx/ 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)
