netdev - Re: [PATCH 08/16] ipvs: fix ip_vs_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20121009014831.GI3403@verge.net.au>
Date:	Tue, 9 Oct 2012 10:48:31 +0900
From:	Simon Horman <horms@...ge.net.au>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	Julian Anastasov <ja@....bg>, Krzysztof Halasa <khc@...waw.pl>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	arm@...nel.org, "David S. Miller" <davem@...emloft.net>,
	netdev@...r.kernel.org, netfilter-devel@...r.kernel.org,
	netfilter@...r.kernel.org, coreteam@...filter.org
Subject: Re: [PATCH 08/16] ipvs: fix ip_vs_set_timeout debug messages

On Sat, Oct 06, 2012 at 09:54:15AM +0000, Arnd Bergmann wrote:
> On Saturday 06 October 2012, Julian Anastasov wrote:
> > On Sat, 6 Oct 2012, Arnd Bergmann wrote:
> > > > 	Are there any CONFIG_IP_VS_PROTO_xxx options in this
> > > > default config? It is a waste of memory if IPVS is compiled
> > > > without any protocols.
> > > 
> > > They all appear to be turned off:
> > > 
> > > $ grep CONFIG_IP_VS obj-tmp/.config
> > > CONFIG_IP_VS=m
> > > CONFIG_IP_VS_DEBUG=y
> > > CONFIG_IP_VS_TAB_BITS=12
> > > # CONFIG_IP_VS_PROTO_TCP is not set
> > > # CONFIG_IP_VS_PROTO_UDP is not set
> > > # CONFIG_IP_VS_PROTO_AH_ESP is not set
> > > # CONFIG_IP_VS_PROTO_ESP is not set
> > > # CONFIG_IP_VS_PROTO_AH is not set
> > > # CONFIG_IP_VS_PROTO_SCTP is not set
> > 
> > 	Something should be changed here, may be at least
> > TCP/UDP, who knows.
> 
> I don't try to read too much into our defconfigs. We have 140 of them
> on ARM, and they are mainly useful to give a reasonable build coverage,
> but I wouldn't expect them to be actually used on that hardware.
> 
> I'll leave it up to Krzysztof to send a patch for this if he wants.
> 
> > > --- a/net/netfilter/ipvs/ip_vs_ctl.c
> > > +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> > > @@ -2590,6 +2588,7 @@ __ip_vs_get_timeouts(struct net *net, struct ip_vs_timeout_user *u)
> > >  #if defined(CONFIG_IP_VS_PROTO_TCP) || defined(CONFIG_IP_VS_PROTO_UDP)
> > >  	struct ip_vs_proto_data *pd;
> > >  #endif
> > 
> > 	That is what we want. If you plan another submission
> > you can add empty line before this memset and to replace
> > the __ip_vs_get_timeouts call in ip_vs_genl_set_config with
> > memset but they are cosmetic changes. Or may be Simon will
> > take care about the coding style when applying the change.
> > 
> > Acked-by: Julian Anastasov <ja@....bg>
> 
> I'd prefer Simon to pick up the patch. He should also decide whether he wants
> to add it to stable. In theory, this is a small leak of kernel stack data
> to user space, but as you say in practice it should not happen because it
> only exists for silly configurations that nobody should be using.
> 
> AFAICT, removing the call to __ip_vs_get_timeouts in do_ip_vs_get_ctl would
> be a semantic change for the case where a user sends a IPVS_CMD_SET_CONFIG
> message without without the complete set of attributes inside it. The current
> behavior is to leave the timeouts alone, replacing the __ip_vs_get_timeouts
> with a memset would zero them. I left this part alone then.
> 
> 	Arnd

Hi,

sorry for being a bit slow, it was a long weekend here.
This patch looks reasonable and I think it is appropriate for stable.
I'll see about getting it merged accordingly.

> 
> 8<-----
> ipvs: initialize returned data in do_ip_vs_get_ctl
> 
> As reported by a gcc warning, the do_ip_vs_get_ctl does not initalize
> all the members of the ip_vs_timeout_user structure it returns if
> at least one of the TCP or UDP protocols is disabled for ipvs. 
> 
> This makes sure that the data is always initialized, before it is
> returned as a response to IPVS_CMD_GET_CONFIG or printed as a
> debug message in IPVS_CMD_SET_CONFIG.
> 
> Without this patch, building ARM ixp4xx_defconfig results in:
> 
> net/netfilter/ipvs/ip_vs_ctl.c: In function 'ip_vs_genl_set_cmd':
> net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.udp_timeout' may be used uninitialized in this function [-Wuninitialized]
> net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.udp_timeout' was declared here
> net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.tcp_fin_timeout' may be used uninitialized in this function [-Wuninitialized]
> net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.tcp_fin_timeout' was declared here
> net/netfilter/ipvs/ip_vs_ctl.c:2238:47: warning: 't.tcp_timeout' may be used uninitialized in this function [-Wuninitialized]
> net/netfilter/ipvs/ip_vs_ctl.c:3322:28: note: 't.tcp_timeout' was declared here
> 
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> Acked-by: Julian Anastasov <ja@....bg>
> ---
> diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
> index 2770f85..c4ee437 100644
> --- a/net/netfilter/ipvs/ip_vs_ctl.c
> +++ b/net/netfilter/ipvs/ip_vs_ctl.c
> @@ -2591,6 +2589,8 @@ __ip_vs_get_timeouts(struct net *net, struct ip_vs_timeout_user *u)
>  	struct ip_vs_proto_data *pd;
>  #endif
>  
> +	memset(u, 0, sizeof (*u));
> +
>  #ifdef CONFIG_IP_VS_PROTO_TCP
>  	pd = ip_vs_proto_data_get(net, IPPROTO_TCP);
>  	u->tcp_timeout = pd->timeout_table[IP_VS_TCP_S_ESTABLISHED] / HZ;
> @@ -2768,7 +2768,6 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
>  	{
>  		struct ip_vs_timeout_user t;
>  
> -		memset(&t, 0, sizeof(t));
>  		__ip_vs_get_timeouts(net, &t);
>  		if (copy_to_user(user, &t, sizeof(t)) != 0)
>  			ret = -EFAULT;
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html