| Message ID | 6dddcc45-78db-4659-80a2-3a2758f491a6@stanley.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ipvs: prevent integer overflow in do_ip_vs_get_ctl() | expand |
On Friday 2025-03-07 14:44, Dan Carpenter wrote: > case IP_VS_SO_GET_SERVICES: > { > struct ip_vs_get_services *get; >- int size; >+ size_t size; > > get = (struct ip_vs_get_services *)arg; > size = struct_size(get, entrytable, get->num_services); > if (*len != size) { >- pr_err("length: %u != %u\n", *len, size); >+ pr_err("length: %u != %lu\n", *len, size); size_t wants %z not %l.
Hello, On Fri, 7 Mar 2025, Dan Carpenter wrote: > The get->num_services variable is an unsigned int which is controlled by > the user. The struct_size() function ensures that the size calculation > does not overflow an unsigned long, however, we are saving the result to > an int so the calculation can overflow. > > Save the result from struct_size() type size_t to fix this integer > overflow bug. > > Cc: stable@vger.kernel.org > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > net/netfilter/ipvs/ip_vs_ctl.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > index 7d13110ce188..801d65fd8a81 100644 > --- a/net/netfilter/ipvs/ip_vs_ctl.c > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > @@ -3091,12 +3091,12 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) > case IP_VS_SO_GET_SERVICES: > { > struct ip_vs_get_services *get; > - int size; > + size_t size; > > get = (struct ip_vs_get_services *)arg; > size = struct_size(get, entrytable, get->num_services); Both are GET operations. The problem that can happen only on 64-bit platforms is that user will attempt copy_to_user() with shorter buffer and will get EFAULT if there are so many entries to return. On 32-bit size will be -1 and will not match *len (EINVAL). So, I assume the issue is not critical, right? > if (*len != size) { > - pr_err("length: %u != %u\n", *len, size); > + pr_err("length: %u != %lu\n", *len, size); %zu, %lu fails on 32-bit platforms. Please, send v2 fixing the format. > ret = -EINVAL; > goto out; > } > @@ -3132,12 +3132,12 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) > case IP_VS_SO_GET_DESTS: > { > struct ip_vs_get_dests *get; > - int size; > + size_t size; > > get = (struct ip_vs_get_dests *)arg; > size = struct_size(get, entrytable, get->num_dests); > if (*len != size) { > - pr_err("length: %u != %u\n", *len, size); > + pr_err("length: %u != %lu\n", *len, size); > ret = -EINVAL; > goto out; > } > -- > 2.47.2 Regards -- Julian Anastasov <ja@ssi.bg>
Hi Dan, kernel test robot noticed the following build warnings: [auto build test WARNING on net/main] url: https://github.com/intel-lab-lkp/linux/commits/Dan-Carpenter/ipvs-prevent-integer-overflow-in-do_ip_vs_get_ctl/20250307-214537 base: net/main patch link: https://lore.kernel.org/r/6dddcc45-78db-4659-80a2-3a2758f491a6%40stanley.mountain patch subject: [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl() config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20250309/202503090225.vNvZGEfz-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250309/202503090225.vNvZGEfz-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202503090225.vNvZGEfz-lkp@intel.com/ All warnings (new ones prefixed by >>): >> net/netfilter/ipvs/ip_vs_ctl.c:3099:40: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat] 3099 | pr_err("length: %u != %lu\n", *len, size); | ~~~ ^~~~ | %zu include/linux/printk.h:544:33: note: expanded from macro 'pr_err' 544 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ~~~ ^~~~~~~~~~~ include/linux/printk.h:501:60: note: expanded from macro 'printk' 501 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) | ~~~ ^~~~~~~~~~~ include/linux/printk.h:473:19: note: expanded from macro 'printk_index_wrap' 473 | _p_func(_fmt, ##__VA_ARGS__); \ | ~~~~ ^~~~~~~~~~~ net/netfilter/ipvs/ip_vs_ctl.c:3140:40: warning: format specifies type 'unsigned long' but the argument has type 'size_t' (aka 'unsigned int') [-Wformat] 3140 | pr_err("length: %u != %lu\n", *len, size); | ~~~ ^~~~ | %zu include/linux/printk.h:544:33: note: expanded from macro 'pr_err' 544 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ~~~ ^~~~~~~~~~~ include/linux/printk.h:501:60: note: expanded from macro 'printk' 501 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) | ~~~ ^~~~~~~~~~~ include/linux/printk.h:473:19: note: expanded from macro 'printk_index_wrap' 473 | _p_func(_fmt, ##__VA_ARGS__); \ | ~~~~ ^~~~~~~~~~~ 2 warnings generated. vim +3099 net/netfilter/ipvs/ip_vs_ctl.c 3012 3013 static int 3014 do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) 3015 { 3016 unsigned char arg[MAX_GET_ARGLEN]; 3017 int ret = 0; 3018 unsigned int copylen; 3019 struct net *net = sock_net(sk); 3020 struct netns_ipvs *ipvs = net_ipvs(net); 3021 3022 BUG_ON(!net); 3023 BUILD_BUG_ON(sizeof(arg) > 255); 3024 if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) 3025 return -EPERM; 3026 3027 if (cmd < IP_VS_BASE_CTL || cmd > IP_VS_SO_GET_MAX) 3028 return -EINVAL; 3029 3030 copylen = get_arglen[CMDID(cmd)]; 3031 if (*len < (int) copylen) { 3032 IP_VS_DBG(1, "get_ctl: len %d < %u\n", *len, copylen); 3033 return -EINVAL; 3034 } 3035 3036 if (copy_from_user(arg, user, copylen) != 0) 3037 return -EFAULT; 3038 /* 3039 * Handle daemons first since it has its own locking 3040 */ 3041 if (cmd == IP_VS_SO_GET_DAEMON) { 3042 struct ip_vs_daemon_user d[2]; 3043 3044 memset(&d, 0, sizeof(d)); 3045 mutex_lock(&ipvs->sync_mutex); 3046 if (ipvs->sync_state & IP_VS_STATE_MASTER) { 3047 d[0].state = IP_VS_STATE_MASTER; 3048 strscpy(d[0].mcast_ifn, ipvs->mcfg.mcast_ifn, 3049 sizeof(d[0].mcast_ifn)); 3050 d[0].syncid = ipvs->mcfg.syncid; 3051 } 3052 if (ipvs->sync_state & IP_VS_STATE_BACKUP) { 3053 d[1].state = IP_VS_STATE_BACKUP; 3054 strscpy(d[1].mcast_ifn, ipvs->bcfg.mcast_ifn, 3055 sizeof(d[1].mcast_ifn)); 3056 d[1].syncid = ipvs->bcfg.syncid; 3057 } 3058 if (copy_to_user(user, &d, sizeof(d)) != 0) 3059 ret = -EFAULT; 3060 mutex_unlock(&ipvs->sync_mutex); 3061 return ret; 3062 } 3063 3064 mutex_lock(&__ip_vs_mutex); 3065 switch (cmd) { 3066 case IP_VS_SO_GET_VERSION: 3067 { 3068 char buf[64]; 3069 3070 sprintf(buf, "IP Virtual Server version %d.%d.%d (size=%d)", 3071 NVERSION(IP_VS_VERSION_CODE), ip_vs_conn_tab_size); 3072 if (copy_to_user(user, buf, strlen(buf)+1) != 0) { 3073 ret = -EFAULT; 3074 goto out; 3075 } 3076 *len = strlen(buf)+1; 3077 } 3078 break; 3079 3080 case IP_VS_SO_GET_INFO: 3081 { 3082 struct ip_vs_getinfo info; 3083 info.version = IP_VS_VERSION_CODE; 3084 info.size = ip_vs_conn_tab_size; 3085 info.num_services = ipvs->num_services; 3086 if (copy_to_user(user, &info, sizeof(info)) != 0) 3087 ret = -EFAULT; 3088 } 3089 break; 3090 3091 case IP_VS_SO_GET_SERVICES: 3092 { 3093 struct ip_vs_get_services *get; 3094 size_t size; 3095 3096 get = (struct ip_vs_get_services *)arg; 3097 size = struct_size(get, entrytable, get->num_services); 3098 if (*len != size) { > 3099 pr_err("length: %u != %lu\n", *len, size); 3100 ret = -EINVAL; 3101 goto out; 3102 } 3103 ret = __ip_vs_get_service_entries(ipvs, get, user); 3104 } 3105 break; 3106 3107 case IP_VS_SO_GET_SERVICE: 3108 { 3109 struct ip_vs_service_entry *entry; 3110 struct ip_vs_service *svc; 3111 union nf_inet_addr addr; 3112 3113 entry = (struct ip_vs_service_entry *)arg; 3114 addr.ip = entry->addr; 3115 rcu_read_lock(); 3116 if (entry->fwmark) 3117 svc = __ip_vs_svc_fwm_find(ipvs, AF_INET, entry->fwmark); 3118 else 3119 svc = __ip_vs_service_find(ipvs, AF_INET, 3120 entry->protocol, &addr, 3121 entry->port); 3122 rcu_read_unlock(); 3123 if (svc) { 3124 ip_vs_copy_service(entry, svc); 3125 if (copy_to_user(user, entry, sizeof(*entry)) != 0) 3126 ret = -EFAULT; 3127 } else 3128 ret = -ESRCH; 3129 } 3130 break; 3131 3132 case IP_VS_SO_GET_DESTS: 3133 { 3134 struct ip_vs_get_dests *get; 3135 size_t size; 3136 3137 get = (struct ip_vs_get_dests *)arg; 3138 size = struct_size(get, entrytable, get->num_dests); 3139 if (*len != size) { 3140 pr_err("length: %u != %lu\n", *len, size); 3141 ret = -EINVAL; 3142 goto out; 3143 } 3144 ret = __ip_vs_get_dest_entries(ipvs, get, user); 3145 } 3146 break; 3147 3148 case IP_VS_SO_GET_TIMEOUT: 3149 { 3150 struct ip_vs_timeout_user t; 3151 3152 __ip_vs_get_timeouts(ipvs, &t); 3153 if (copy_to_user(user, &t, sizeof(t)) != 0) 3154 ret = -EFAULT; 3155 } 3156 break; 3157 3158 default: 3159 ret = -EINVAL; 3160 } 3161 3162 out: 3163 mutex_unlock(&__ip_vs_mutex); 3164 return ret; 3165 } 3166
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index 7d13110ce188..801d65fd8a81 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -3091,12 +3091,12 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) case IP_VS_SO_GET_SERVICES: { struct ip_vs_get_services *get; - int size; + size_t size; get = (struct ip_vs_get_services *)arg; size = struct_size(get, entrytable, get->num_services); if (*len != size) { - pr_err("length: %u != %u\n", *len, size); + pr_err("length: %u != %lu\n", *len, size); ret = -EINVAL; goto out; } @@ -3132,12 +3132,12 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) case IP_VS_SO_GET_DESTS: { struct ip_vs_get_dests *get; - int size; + size_t size; get = (struct ip_vs_get_dests *)arg; size = struct_size(get, entrytable, get->num_dests); if (*len != size) { - pr_err("length: %u != %u\n", *len, size); + pr_err("length: %u != %lu\n", *len, size); ret = -EINVAL; goto out; }
The get->num_services variable is an unsigned int which is controlled by the user. The struct_size() function ensures that the size calculation does not overflow an unsigned long, however, we are saving the result to an int so the calculation can overflow. Save the result from struct_size() type size_t to fix this integer overflow bug. Cc: stable@vger.kernel.org Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- net/netfilter/ipvs/ip_vs_ctl.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)