[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Alex Gartrell]

Ensure that the pointer is non-NULL before dereferencing it for debugging
purposes.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
 net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 91f17c1..06bba9b 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
 	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
 						  local))) {
 		IP_VS_DBG_RL("We are crossing local and non-local addresses"
-			     " daddr=%pI4\n", &dest->addr.ip);
+			     " daddr=%pI4\n", dest ? &dest->addr.ip : NULL);
 		goto err_put;
 	}
 
@@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
 	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
 						  local))) {
 		IP_VS_DBG_RL("We are crossing local and non-local addresses"
-			     " daddr=%pI6\n", &dest->addr.in6);
+			     " daddr=%pI6\n", dest ? &dest->addr.in6 : NULL);
 		goto err_put;
 	}
 
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Julian Anastasov]

	Hello,

On Sun, 5 Oct 2014, Alex Gartrell wrote:

> Ensure that the pointer is non-NULL before dereferencing it for debugging
> purposes.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Alex Gartrell <agartrell@fb.com>
> ---
>  net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index 91f17c1..06bba9b 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
>  	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
>  						  local))) {
>  		IP_VS_DBG_RL("We are crossing local and non-local addresses"
> -			     " daddr=%pI4\n", &dest->addr.ip);
> +			     " daddr=%pI4\n", dest ? &dest->addr.ip : NULL);
>  		goto err_put;
>  	}
>  
> @@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
>  	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
>  						  local))) {
>  		IP_VS_DBG_RL("We are crossing local and non-local addresses"
> -			     " daddr=%pI6\n", &dest->addr.in6);
> +			     " daddr=%pI6\n", dest ? &dest->addr.in6 : NULL);
>  		goto err_put;
>  	}

	You have to print the "daddr" variable as
it was done before your patchset in the
"Stopping traffic to %s address, dest: %p..." message
because dest is not present in all cases, for example,
for *bypass_xmit. Other places provide cp->daddr but
for backup server some conns can live without cp->dest.

Regards

--
Julian Anastasov <ja@ssi.bg>

[PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Alex Gartrell]

Use daddr instead of reaching into dest.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Alex Gartrell <agartrell@fb.com>
---
 net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
index 91f17c1..437a366 100644
--- a/net/netfilter/ipvs/ip_vs_xmit.c
+++ b/net/netfilter/ipvs/ip_vs_xmit.c
@@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
 	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
 						  local))) {
 		IP_VS_DBG_RL("We are crossing local and non-local addresses"
-			     " daddr=%pI4\n", &dest->addr.ip);
+			     " daddr=%pI4\n", &daddr);
 		goto err_put;
 	}
 
@@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
 	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
 						  local))) {
 		IP_VS_DBG_RL("We are crossing local and non-local addresses"
-			     " daddr=%pI6\n", &dest->addr.in6);
+			     " daddr=%pI6\n", daddr);
 		goto err_put;
 	}
 
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Alex Gartrell]

Hey Julian,

On 10/05/2014 11:49 PM, Julian Anastasov wrote:
>
> 	You have to print the "daddr" variable as
> it was done before your patchset in the
> "Stopping traffic to %s address, dest: %p..." message
> because dest is not present in all cases, for example,
> for *bypass_xmit. Other places provide cp->daddr but
> for backup server some conns can live without cp->dest.

I've sent an updated patch that does this but I have some questions 
about other stuff that I find mildly confusing.  Specifically I didn't 
realize until looking at the call sites that !dest || daddr = 
dest->addr.ip (but maybe I'm wrong?)

If that's the case, why do we have the following line in __ip_vs_get_out_rt?

                 daddr = dest->addr.ip;

If that's /always/ true then we should add the following line or a 
comment to the same effect to clarify

	BUG_ON(dest && dest->addr.ip != daddr);

If that's not intended to always be true, then should the patch be the 
following?

	...%pI4", dest ? &dest->addr.ip : &daddr);

Thanks,
-- 
Alex Gartrell <agartrell@fb.com>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Julian Anastasov]

	Hello,

On Mon, 6 Oct 2014, Alex Gartrell wrote:

> Use daddr instead of reaching into dest.
> 
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Alex Gartrell <agartrell@fb.com>

	Thanks!

Acked-by: Julian Anastasov <ja@ssi.bg>

> ---
>  net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> index 91f17c1..437a366 100644
> --- a/net/netfilter/ipvs/ip_vs_xmit.c
> +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> @@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
>  	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
>  						  local))) {
>  		IP_VS_DBG_RL("We are crossing local and non-local addresses"
> -			     " daddr=%pI4\n", &dest->addr.ip);
> +			     " daddr=%pI4\n", &daddr);
>  		goto err_put;
>  	}
>  
> @@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
>  	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
>  						  local))) {
>  		IP_VS_DBG_RL("We are crossing local and non-local addresses"
> -			     " daddr=%pI6\n", &dest->addr.in6);
> +			     " daddr=%pI6\n", daddr);
>  		goto err_put;
>  	}
>  
> -- 
> Alex Gartrell <agartrell@fb.com>

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Julian Anastasov]

	Hello,

On Mon, 6 Oct 2014, Alex Gartrell wrote:

> Hey Julian,
> 
> I've sent an updated patch that does this but I have some questions about
> other stuff that I find mildly confusing.  Specifically I didn't realize until
> looking at the call sites that !dest || daddr = dest->addr.ip (but maybe I'm
> wrong?)
> 
> If that's the case, why do we have the following line in __ip_vs_get_out_rt?
> 
>                 daddr = dest->addr.ip;

	Extra line that is not needed...

> If that's /always/ true then we should add the following line or a comment to
> the same effect to clarify
> 
> 	BUG_ON(dest && dest->addr.ip != daddr);

	IMHO, BUG_ON is not needed.

> If that's not intended to always be true, then should the patch be the
> following?
> 
> 	...%pI4", dest ? &dest->addr.ip : &daddr);

	Using daddr is fine.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [PATCH net-next] ipvs: Avoid null-pointer deref in debug code

[Simon Horman]

On Mon, Oct 06, 2014 at 10:01:08PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 6 Oct 2014, Alex Gartrell wrote:
> 
> > Use daddr instead of reaching into dest.
> > 
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Alex Gartrell <agartrell@fb.com>
> 
> 	Thanks!
> 
> Acked-by: Julian Anastasov <ja@ssi.bg>

Thanks, I have applied this to the ipvs tree and will see about
both getting it included in v3.18 and v3.17-stable.

> > ---
> >  net/netfilter/ipvs/ip_vs_xmit.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/netfilter/ipvs/ip_vs_xmit.c b/net/netfilter/ipvs/ip_vs_xmit.c
> > index 91f17c1..437a366 100644
> > --- a/net/netfilter/ipvs/ip_vs_xmit.c
> > +++ b/net/netfilter/ipvs/ip_vs_xmit.c
> > @@ -316,7 +316,7 @@ __ip_vs_get_out_rt(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> >  	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> >  						  local))) {
> >  		IP_VS_DBG_RL("We are crossing local and non-local addresses"
> > -			     " daddr=%pI4\n", &dest->addr.ip);
> > +			     " daddr=%pI4\n", &daddr);
> >  		goto err_put;
> >  	}
> >  
> > @@ -458,7 +458,7 @@ __ip_vs_get_out_rt_v6(int skb_af, struct sk_buff *skb, struct ip_vs_dest *dest,
> >  	if (unlikely(crosses_local_route_boundary(skb_af, skb, rt_mode,
> >  						  local))) {
> >  		IP_VS_DBG_RL("We are crossing local and non-local addresses"
> > -			     " daddr=%pI6\n", &dest->addr.in6);
> > +			     " daddr=%pI6\n", daddr);
> >  		goto err_put;
> >  	}
> >  
> > -- 
> > Alex Gartrell <agartrell@fb.com>
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>
>