* Re: [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl()
2025-03-07 13:44 [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl() Dan Carpenter
@ 2025-03-07 14:06 ` Jan Engelhardt
2025-03-08 12:06 ` Julian Anastasov
2025-03-08 19:03 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: Jan Engelhardt @ 2025-03-07 14:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gustavo A. R. Silva, Simon Horman, Julian Anastasov,
Pablo Neira Ayuso, Jozsef Kadlecsik, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev, lvs-devel,
netfilter-devel, coreteam, linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl()
2025-03-07 13:44 [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl() Dan Carpenter
2025-03-07 14:06 ` Jan Engelhardt
@ 2025-03-08 12:06 ` Julian Anastasov
2025-03-08 19:03 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: Julian Anastasov @ 2025-03-08 12:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: Gustavo A. R. Silva, Simon Horman, Pablo Neira Ayuso,
Jozsef Kadlecsik, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, lvs-devel, netfilter-devel, coreteam,
linux-kernel, kernel-janitors
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>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl()
2025-03-07 13:44 [PATCH net] ipvs: prevent integer overflow in do_ip_vs_get_ctl() Dan Carpenter
2025-03-07 14:06 ` Jan Engelhardt
2025-03-08 12:06 ` Julian Anastasov
@ 2025-03-08 19:03 ` kernel test robot
2 siblings, 0 replies; 4+ messages in thread
From: kernel test robot @ 2025-03-08 19:03 UTC (permalink / raw)
To: Dan Carpenter, Gustavo A. R. Silva
Cc: llvm, oe-kbuild-all, Simon Horman, Julian Anastasov,
Pablo Neira Ayuso, Jozsef Kadlecsik, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, lvs-devel, netfilter-devel, coreteam,
linux-kernel, kernel-janitors
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 4+ messages in thread