include/net/netlink.h | 2 ++ 1 file changed, 2 insertions(+)
The "payload" variable is type size_t, however the nlmsg_total_size()
function will a few bytes to it and then truncate the result to type
int. That means that if "payload" is more than UINT_MAX the alloc_skb()
function might allocate a buffer which is smaller than intended.
Cc: stable@vger.kernel.org
Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
include/net/netlink.h | 2 ++
1 file changed, 2 insertions(+)
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);
}
--
2.45.2On Wed, Jan 22, 2025 at 04:49:17PM +0300, Dan Carpenter wrote:
> The "payload" variable is type size_t, however the nlmsg_total_size()
> function will a few bytes to it and then truncate the result to type
> int. That means that if "payload" is more than UINT_MAX the alloc_skb()
> function might allocate a buffer which is smaller than intended.
>
> Cc: stable@vger.kernel.org
> Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> include/net/netlink.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> 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);
Hi Dan,
I wonder if this is sufficient.
If payload is INT_MAX then won't the call to nlmsg_msg_size() inside
nlmsg_total_size() overflow. And likewise, it feels that NLMSG_ALIGN
could overflow somehow.
On Wed, 22 Jan 2025 16:49:17 +0300 Dan Carpenter wrote: > The "payload" variable is type size_t, however the nlmsg_total_size() > function will a few bytes to it and then truncate the result to type > int. That means that if "payload" is more than UINT_MAX the alloc_skb() > function might allocate a buffer which is smaller than intended. Is there a bug, or is this theoretical?
On Wed, Jan 22, 2025 at 06:24:27AM -0800, Jakub Kicinski wrote:
> On Wed, 22 Jan 2025 16:49:17 +0300 Dan Carpenter wrote:
> > The "payload" variable is type size_t, however the nlmsg_total_size()
> > function will a few bytes to it and then truncate the result to type
> > int. That means that if "payload" is more than UINT_MAX the alloc_skb()
> > function might allocate a buffer which is smaller than intended.
>
> Is there a bug, or is this theoretical?
The rule here is that if we pass something very close to UINT_MAX to
nlmsg_new() the it leads to an integer overflow. I'm not a networking
expert. The caller that concerned me was:
*** 1 ***
net/netfilter/ipset/ip_set_core.c
1762 /* Error in restore/batch mode: send back lineno */
1763 struct nlmsghdr *rep, *nlh = nlmsg_hdr(skb);
1764 struct sk_buff *skb2;
1765 struct nlmsgerr *errmsg;
1766 size_t payload = min(SIZE_MAX,
1767 sizeof(*errmsg) + nlmsg_len(nlh));
I don't know the limits of limits of nlmsg_len() here.
The min(SIZE_MAX is what scared me. That was added to silence a Smatch
warning. :P It should be fixed or removed.
1768 int min_len = nlmsg_total_size(sizeof(struct nfgenmsg));
1769 struct nlattr *cda[IPSET_ATTR_CMD_MAX + 1];
1770 struct nlattr *cmdattr;
1771 u32 *errline;
1772
1773 skb2 = nlmsg_new(payload, GFP_KERNEL);
1774 if (!skb2)
1775 return -ENOMEM;
*** 2 ***
There is similar code in netlink_ack() where the payload comes from
nlmsg_len(nlh).
*** 3 ***
There is a potential issue in queue_userspace_packet() when we call:
len = upcall_msg_size(upcall_info, hlen - cutlen, ...
^^^^^^^^^^^^^
user_skb = genlmsg_new(len, GFP_ATOMIC);
It's possible that hlen is less than cutlen. (That's a separate bug,
I'll send a fix for it).
regards,
dan carpenter
On Fri, 24 Jan 2025 17:35:24 +0300 Dan Carpenter wrote: > On Wed, Jan 22, 2025 at 06:24:27AM -0800, Jakub Kicinski wrote: > > On Wed, 22 Jan 2025 16:49:17 +0300 Dan Carpenter wrote: > > > The "payload" variable is type size_t, however the nlmsg_total_size() > > > function will a few bytes to it and then truncate the result to type > > > int. That means that if "payload" is more than UINT_MAX the alloc_skb() > > > function might allocate a buffer which is smaller than intended. > > > > Is there a bug, or is this theoretical? > > The rule here is that if we pass something very close to UINT_MAX to > nlmsg_new() the it leads to an integer overflow. I'm not a networking > expert. The caller that concerned me was: > > *** 1 *** > > net/netfilter/ipset/ip_set_core.c > 1762 /* Error in restore/batch mode: send back lineno */ > 1763 struct nlmsghdr *rep, *nlh = nlmsg_hdr(skb); > 1764 struct sk_buff *skb2; > 1765 struct nlmsgerr *errmsg; > 1766 size_t payload = min(SIZE_MAX, > 1767 sizeof(*errmsg) + nlmsg_len(nlh)); > > I don't know the limits of limits of nlmsg_len() here. Practically speaking the limits are fairly small. The nlh comes from user's request / sendmsg() call. So the user must have prepared a message of at least that len, and kernel must had been able to kvmalloc() a linear buffer large enough to copy that message in. > The min(SIZE_MAX is what scared me. That was added to silence a Smatch > warning. :P It should be fixed or removed. Yeah, that ip_set code looks buggy. Mostly because we use @payload for the nlmsg_put() call, but then raw nlh->nlmsg_len for memcpy() :S > 1768 int min_len = nlmsg_total_size(sizeof(struct nfgenmsg)); > 1769 struct nlattr *cda[IPSET_ATTR_CMD_MAX + 1]; > 1770 struct nlattr *cmdattr; > 1771 u32 *errline; > 1772 > 1773 skb2 = nlmsg_new(payload, GFP_KERNEL); > 1774 if (!skb2) > 1775 return -ENOMEM; > > *** 2 *** > There is similar code in netlink_ack() where the payload comes from > nlmsg_len(nlh). This one is correct. Each piece of the message is nlmsg_put() individually, which does bounds checking. So if the allocation of the skb was faulty and the skb is shorter than we expected we'll just error out on the put. > *** 3 *** > > There is a potential issue in queue_userspace_packet() when we call: > > len = upcall_msg_size(upcall_info, hlen - cutlen, ... > ^^^^^^^^^^^^^ > user_skb = genlmsg_new(len, GFP_ATOMIC); > > It's possible that hlen is less than cutlen. (That's a separate bug, > I'll send a fix for it). Ack. In general IMVHO the check in nlmsg_new() won't be too effective. The callers can overflow their local message size calculation. Not to mention that the size calculation is often inexact. So using nla_put() and checking error codes is the best way to prevent security issues..
On 1/22/25 14:49, Dan Carpenter wrote:
> The "payload" variable is type size_t, however the nlmsg_total_size()
> function will a few bytes to it and then truncate the result to type
> int. That means that if "payload" is more than UINT_MAX the alloc_skb()
In the code it's INT_MAX, would be best to have the same used in both
places (or explain it so it's obvious)
> function might allocate a buffer which is smaller than intended.
>
> Cc: stable@vger.kernel.org
> Fixes: bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> include/net/netlink.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> 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);
> }
>
On Wed, Jan 22, 2025 at 02:52:39PM +0100, Przemek Kitszel wrote:
> On 1/22/25 14:49, Dan Carpenter wrote:
> > The "payload" variable is type size_t, however the nlmsg_total_size()
> > function will a few bytes to it and then truncate the result to type
> > int. That means that if "payload" is more than UINT_MAX the alloc_skb()
>
> In the code it's INT_MAX, would be best to have the same used in both
> places (or explain it so it's obvious)
>
Yeah. It's not probably not obvious.
I don't like using UINT_MAX as a limit because why push so close to the
edge? Normal allocation functions are capped at INT_MAX to avoid
integer overflows. You'd have to use vmalloc() to allocate more than
2GB of RAM. So it's not like we gain anything by using a higher, riskier
number.
The nlmsg_total_size() function adds potentially 19 bytes to the
payload.
INT_MAX plus anything less than 2 million number can't overflow to zero.
It could overflow to negative but you can't allocate negative bytes so
that's fine.
The vfs_read/write() functions use MAX_RW_COUNT to avoid integer
overflows. That's basically INT_MAX - PAGE_SIZE. There are quite
a few places like this in the kernel which assume small numbers like
sizeof() are generally going to return less than PAGE_SIZE. Would
that be better to do this. Then it couldn't overflow to negative.
regards,
dan carpenter
diff --git a/include/net/netlink.h b/include/net/netlink.h
index e015ffbed819..ceeea04fae4a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1015,6 +1015,9 @@ static inline struct nlmsghdr *nlmsg_put_answer(struct sk_buff *skb,
*/
static inline struct sk_buff *nlmsg_new(size_t payload, gfp_t flags)
{
+ /* Prevent integer overflow */
+ if (payload > INT_MAX - PAGE_SIZE)
+ return NULL;
return alloc_skb(nlmsg_total_size(payload), flags);
}
© 2016 - 2026 Red Hat, Inc.