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..
