| Message ID | 58023f9e-555e-48db-9822-283c2c1f6d0e@stanley.mountain (mailing list archive) |
|---|---|
| State | New |
| 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); }
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(+)