| Message ID | 92dc4619-7598-439e-8544-4b3b2cf5e597@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] xfrm: prevent some integer overflows in verify_ functions | expand |
On Tue, Dec 17, 2024 at 11:42:31AM +0300, Dan Carpenter wrote: > > + if (algp->alg_key_len > INT_MAX) { Why not check for UINT_MAX - 7? INT_MAX seems a bit arbitrary. Thanks,
On Tue, Dec 17, 2024 at 08:03:38PM +0800, Herbert Xu wrote: > On Tue, Dec 17, 2024 at 11:42:31AM +0300, Dan Carpenter wrote: > > > > + if (algp->alg_key_len > INT_MAX) { > > Why not check for UINT_MAX - 7? INT_MAX seems a bit arbitrary. > That seems like basic algebra but we have a long history of getting integer overflow checks wrong so these days I like to just use INT_MAX where ever I can. I wanted to use USHRT_MAX. We aren't allowed to use more than USHRT_MAX bytes, but maybe we're allowed USHRT_MAX bits, so I didn't do that. regards, dan carpenter
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index 08c6d6f0179f..686c6a24d92b 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -45,6 +45,10 @@ static int verify_one_alg(struct nlattr **attrs, enum xfrm_attr_type_t type, return 0; algp = nla_data(rt); + if (algp->alg_key_len > INT_MAX) { + NL_SET_ERR_MSG(extack, "Invalid AUTH/CRYPT/COMP key length"); + return -EINVAL; + } if (nla_len(rt) < (int)xfrm_alg_len(algp)) { NL_SET_ERR_MSG(extack, "Invalid AUTH/CRYPT/COMP attribute length"); return -EINVAL; @@ -75,6 +79,10 @@ static int verify_auth_trunc(struct nlattr **attrs, return 0; algp = nla_data(rt); + if (algp->alg_key_len > INT_MAX) { + NL_SET_ERR_MSG(extack, "Invalid AUTH_TRUNC key length"); + return -EINVAL; + } if (nla_len(rt) < (int)xfrm_alg_auth_len(algp)) { NL_SET_ERR_MSG(extack, "Invalid AUTH_TRUNC attribute length"); return -EINVAL; @@ -93,6 +101,10 @@ static int verify_aead(struct nlattr **attrs, struct netlink_ext_ack *extack) return 0; algp = nla_data(rt); + if (algp->alg_key_len > INT_MAX) { + NL_SET_ERR_MSG(extack, "Invalid AEAD key length"); + return -EINVAL; + } if (nla_len(rt) < (int)aead_len(algp)) { NL_SET_ERR_MSG(extack, "Invalid AEAD attribute length"); return -EINVAL;
The xfrm_alg_len() type functions take the alg->alg_key_len which is the length in bits and converts it to bytes and add it to sizeof(*alg). return sizeof(*alg) + ((alg->alg_key_len + 7) / 8); The alg->alg_key_len is type unsigned int. That means that if we pick an ->alg_key_len which is greater than "UINT_MAX - 7" it leads to an integer overflow and the key length is treated as zero. The result is that xfrm_alg_len() function will return "sizeof(*alg) + 0". However, so far as I can see this does not cause a problem. All the places which use this length consistently do the same conversion. The type of thing I was looking for would be code which uses partial keys or code which uses a different type instead of u32 for ->alg_key_len. I didn't find anything like that so I can't see a negative impact from this bug. Still fixing it is the right thing to do. Fixes: 31c26852cb2a ("[IPSEC]: Verify key payload in verify_one_algo") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- net/xfrm/xfrm_user.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)