| Message ID | 58023f9e-555e-48db-9822-283c2c1f6d0e@stanley.mountain (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: netlink: prevent potential integer overflow in nlmsg_new() | expand |
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, 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 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, 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); }
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..
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); }
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(+)