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
| ||
|
Date: Thu, 28 Jan 2016 08:39:14 +0900 From: Simon Horman <horms@...ge.net.au> To: Julian Anastasov <ja@....bg> Cc: Arnd Bergmann <arnd@...db.de>, Wensong Zhang <wensong@...ux-vs.org>, linux-arm-kernel@...ts.infradead.org, Pablo Neira Ayuso <pablo@...filter.org>, Patrick McHardy <kaber@...sh.net>, Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>, "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org, lvs-devel@...r.kernel.org, netfilter-devel@...r.kernel.org, coreteam@...filter.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 2/2] netfilter: ipvs/SIP: handle ip_vs_fill_iph_skb_off failure On Wed, Jan 27, 2016 at 10:56:08PM +0200, Julian Anastasov wrote: > > Hello, > > On Wed, 27 Jan 2016, Arnd Bergmann wrote: > > > ip_vs_fill_iph_skb_off() may not find an IP header, and gcc has > > determined that ip_vs_sip_fill_param() then incorrectly accesses > > the protocol fields: > > > > net/netfilter/ipvs/ip_vs_pe_sip.c: In function 'ip_vs_sip_fill_param': > > net/netfilter/ipvs/ip_vs_pe_sip.c:76:5: error: 'iph.protocol' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > if (iph.protocol != IPPROTO_UDP) > > ^ > > net/netfilter/ipvs/ip_vs_pe_sip.c:81:10: error: 'iph.len' may be used uninitialized in this function [-Werror=maybe-uninitialized] > > dataoff = iph.len + sizeof(struct udphdr); > > ^ > > > > This adds a check for the ip_vs_fill_iph_skb_off() return code > > before looking at the ip header data returned from it. > > > > Signed-off-by: Arnd Bergmann <arnd@...db.de> > > Fixes: b0e010c527de ("ipvs: replace ip_vs_fill_ip4hdr with ip_vs_fill_iph_skb_off") > > Looks ok to me, > > Acked-by: Julian Anastasov <ja@....bg> Thanks, I have queued this up. > but see below... > > > --- > > net/netfilter/ipvs/ip_vs_pe_sip.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/net/netfilter/ipvs/ip_vs_pe_sip.c b/net/netfilter/ipvs/ip_vs_pe_sip.c > > index 1b8d594e493a..c4e9ca016a88 100644 > > --- a/net/netfilter/ipvs/ip_vs_pe_sip.c > > +++ b/net/netfilter/ipvs/ip_vs_pe_sip.c > > @@ -70,10 +70,10 @@ ip_vs_sip_fill_param(struct ip_vs_conn_param *p, struct sk_buff *skb) > > const char *dptr; > > int retc; > > > > - ip_vs_fill_iph_skb(p->af, skb, false, &iph); > > + retc = ip_vs_fill_iph_skb(p->af, skb, false, &iph); > > > > /* Only useful with UDP */ > > - if (iph.protocol != IPPROTO_UDP) > > + if (!retc || iph.protocol != IPPROTO_UDP) > > return -EINVAL; > > /* todo: IPv6 fragments: > > * I think this only should be done for the first fragment. /HS > > There are other places like this where result is not > checked because there is always a guarding skb_header_pointer > check, i.e. ip_vs_fill_iph_skb* should not fail at such point. > > Let us know you want to extend this patch with other such > calls (including ip_vs_fill_iph_skb_icmp)? May be they will > need return NF_ACCEPT. I guess, all such changes should be > for the ipvs-next/net-next tree when it opens. I would suggest making such changes incrementally on top of this one.
Powered by blists - more mailing lists