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 <a...@arndb.de>
> > Fixes: b0e010c527de ("ipvs: replace ip_vs_fill_ip4hdr with
> > ip_vs_fill_iph_skb_off")
>
> Looks ok to me,
>
> Acked-by: Julian Anastasov <j...@ssi.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.
