[PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Lance Richardson]

Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
before calling fib_lookup(), which means fib_table_lookup() is
using non-deterministic data at this line:

	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {

Fix by initializing fl4.flowi4_flags to zero.

Signed-off-by: Lance Richardson <lrichard@redhat.com>
---
 net/ipv4/fib_frontend.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 21add55..896844a 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
 		fl4.flowi4_scope = scope;
 		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
 		fl4.flowi4_tun_key.tun_id = 0;
+		fl4.flowi4_flags = 0;
 		if (!fib_lookup(net, &fl4, &res, 0))
 			return FIB_RES_PREFSRC(net, res);
 	} else {
-- 
2.5.0

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[David Ahern]

On 3/22/16 9:31 AM, Lance Richardson wrote:
> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
> before calling fib_lookup(), which means fib_table_lookup() is
> using non-deterministic data at this line:
>
> 	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
>
> Fix by initializing fl4.flowi4_flags to zero.
>
> Signed-off-by: Lance Richardson <lrichard@redhat.com>
> ---
>   net/ipv4/fib_frontend.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 21add55..896844a 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>   		fl4.flowi4_scope = scope;
>   		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
>   		fl4.flowi4_tun_key.tun_id = 0;
> +		fl4.flowi4_flags = 0;
>   		if (!fib_lookup(net, &fl4, &res, 0))
>   			return FIB_RES_PREFSRC(net, res);
>   	} else {
>

Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")

I think a more robust solution is to move fl4 to this if case and init 
when it is declared:

	struct flowi4 fl4 = {
		.flowi4_iif = LOOPBACK_IFINDEX,
		.daddr = ip_hdr(skb)->saddr,
		.flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
		.flowi4_scope = scope,
		.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
	};

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Lance Richardson]

----- Original Message -----
> From: "David Ahern" <dsa@cumulusnetworks.com>
> To: "Lance Richardson" <lrichard@redhat.com>, netdev@vger.kernel.org
> Sent: Tuesday, March 22, 2016 2:29:02 PM
> Subject: Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
> 
> On 3/22/16 9:31 AM, Lance Richardson wrote:
> > Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
> > before calling fib_lookup(), which means fib_table_lookup() is
> > using non-deterministic data at this line:
> >
> > 	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
> >
> > Fix by initializing fl4.flowi4_flags to zero.
> >
> > Signed-off-by: Lance Richardson <lrichard@redhat.com>
> > ---
> >   net/ipv4/fib_frontend.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> > index 21add55..896844a 100644
> > --- a/net/ipv4/fib_frontend.c
> > +++ b/net/ipv4/fib_frontend.c
> > @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
> >   		fl4.flowi4_scope = scope;
> >   		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
> >   		fl4.flowi4_tun_key.tun_id = 0;
> > +		fl4.flowi4_flags = 0;
> >   		if (!fib_lookup(net, &fl4, &res, 0))
> >   			return FIB_RES_PREFSRC(net, res);
> >   	} else {
> >
> 
> Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
> 
> I think a more robust solution is to move fl4 to this if case and init
> when it is declared:
> 
> 	struct flowi4 fl4 = {
> 		.flowi4_iif = LOOPBACK_IFINDEX,
> 		.daddr = ip_hdr(skb)->saddr,
> 		.flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
> 		.flowi4_scope = scope,
> 		.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
> 	};
> 

Agreed... I actually debated doing something similar but opted for the
smaller delta.

v2 coming up.

Thanks for the review,

   Lance

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Cong Wang]

On Tue, Mar 22, 2016 at 11:29 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 3/22/16 9:31 AM, Lance Richardson wrote:
>>
>> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
>> before calling fib_lookup(), which means fib_table_lookup() is
>> using non-deterministic data at this line:
>>
>>         if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
>>
>> Fix by initializing fl4.flowi4_flags to zero.
>>
>> Signed-off-by: Lance Richardson <lrichard@redhat.com>
>> ---
>>   net/ipv4/fib_frontend.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
>> index 21add55..896844a 100644
>> --- a/net/ipv4/fib_frontend.c
>> +++ b/net/ipv4/fib_frontend.c
>> @@ -304,6 +304,7 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>>                 fl4.flowi4_scope = scope;
>>                 fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark :
>> 0;
>>                 fl4.flowi4_tun_key.tun_id = 0;
>> +               fl4.flowi4_flags = 0;
>>                 if (!fib_lookup(net, &fl4, &res, 0))
>>                         return FIB_RES_PREFSRC(net, res);
>>         } else {
>>
>
> Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")

Why does this patch fix this commit? Even before this commit, flowi4_flags
was already tested for other bit:


@@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const
struct flowi4 *flp,
                            nh->nh_flags & RTNH_F_LINKDOWN &&
                            !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
                                continue;
-                       if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) {
+                       if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
                                if (flp->flowi4_oif &&
                                    flp->flowi4_oif != nh->nh_oif)
                                        continue;

For me, it looks the bug was introduce by commit 35ebf65e851c6d9731abc6362b1.

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[David Ahern]

On 3/22/16 2:45 PM, Cong Wang wrote:
> @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const
> struct flowi4 *flp,
>                              nh->nh_flags & RTNH_F_LINKDOWN &&
>                              !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
>                                  continue;
> -                       if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) {
> +                       if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
>                                  if (flp->flowi4_oif &&
>                                      flp->flowi4_oif != nh->nh_oif)
>                                          continue;
>
> For me, it looks the bug was introduce by commit 35ebf65e851c6d9731abc6362b1.
>

Arguably yes since it added the function without initializing flags.

The commit I referenced (and even the VRF predecessor both of which 
originated in the v4.3) is the one introducing use of flow flags to the 
lookup. From a stable perspective going back to v4.3 is what matters.

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Cong Wang]

On Tue, Mar 22, 2016 at 2:02 PM, David Ahern <dsa@cumulusnetworks.com> wrote:
> On 3/22/16 2:45 PM, Cong Wang wrote:
>>
>> @@ -1426,7 +1426,7 @@ int fib_table_lookup(struct fib_table *tb, const
>> struct flowi4 *flp,
>>                              nh->nh_flags & RTNH_F_LINKDOWN &&
>>                              !(fib_flags & FIB_LOOKUP_IGNORE_LINKSTATE))
>>                                  continue;
>> -                       if (!(flp->flowi4_flags & FLOWI_FLAG_VRFSRC)) {
>> +                       if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF))
>> {
>>                                  if (flp->flowi4_oif &&
>>                                      flp->flowi4_oif != nh->nh_oif)
>>                                          continue;
>>
>> For me, it looks the bug was introduce by commit
>> 35ebf65e851c6d9731abc6362b1.
>>
>
> Arguably yes since it added the function without initializing flags.
>
> The commit I referenced (and even the VRF predecessor both of which
> originated in the v4.3) is the one introducing use of flow flags to the
> lookup. From a stable perspective going back to v4.3 is what matters.
>

Yeah, then it is commit 613d09b30f8b589d5a9b497. It doesn't matter
for backport, but matters for accuracy. ;)

[PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Lance Richardson]

Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
before calling fib_lookup(), which means fib_table_lookup() is
using non-deterministic data at this line:

	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {

Fix by initializing the entire fl4 structure, which will prevent
similar issues as fields are added in the future by ensuring that
all fields are initialized to zero unless explicitly initialized
to another value.

Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
Suggested-by: David Ahern <dsa@cumulusnetworks.com>
Signed-off-by: Lance Richardson <lrichard@redhat.com>
---
 net/ipv4/fib_frontend.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 21add55..8a9246d 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -280,7 +280,6 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
 	struct in_device *in_dev;
 	struct fib_result res;
 	struct rtable *rt;
-	struct flowi4 fl4;
 	struct net *net;
 	int scope;
 
@@ -296,14 +295,13 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
 
 	scope = RT_SCOPE_UNIVERSE;
 	if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
-		fl4.flowi4_oif = 0;
-		fl4.flowi4_iif = LOOPBACK_IFINDEX;
-		fl4.daddr = ip_hdr(skb)->saddr;
-		fl4.saddr = 0;
-		fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
-		fl4.flowi4_scope = scope;
-		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
-		fl4.flowi4_tun_key.tun_id = 0;
+		struct flowi4 fl4 = {
+			.flowi4_iif = LOOPBACK_IFINDEX,
+			.daddr = ip_hdr(skb)->saddr,
+			.flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
+			.flowi4_scope = scope,
+			.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
+		};
 		if (!fib_lookup(net, &fl4, &res, 0))
 			return FIB_RES_PREFSRC(net, res);
 	} else {
-- 
2.5.5

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Lance Richardson]

Apologies, that should have been [PATCH v2 net].

----- Original Message -----
> From: "Lance Richardson" <lrichard@redhat.com>
> To: netdev@vger.kernel.org
> Cc: dsa@cumulusnetworks.com
> Sent: Tuesday, March 22, 2016 2:56:57 PM
> Subject: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()
> 
> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
> before calling fib_lookup(), which means fib_table_lookup() is
> using non-deterministic data at this line:
> 
> 	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
> 
> Fix by initializing the entire fl4 structure, which will prevent
> similar issues as fields are added in the future by ensuring that
> all fields are initialized to zero unless explicitly initialized
> to another value.
> 
> Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
> Suggested-by: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Lance Richardson <lrichard@redhat.com>
> ---
>  net/ipv4/fib_frontend.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 21add55..8a9246d 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -280,7 +280,6 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>  	struct in_device *in_dev;
>  	struct fib_result res;
>  	struct rtable *rt;
> -	struct flowi4 fl4;
>  	struct net *net;
>  	int scope;
>  
> @@ -296,14 +295,13 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>  
>  	scope = RT_SCOPE_UNIVERSE;
>  	if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
> -		fl4.flowi4_oif = 0;
> -		fl4.flowi4_iif = LOOPBACK_IFINDEX;
> -		fl4.daddr = ip_hdr(skb)->saddr;
> -		fl4.saddr = 0;
> -		fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
> -		fl4.flowi4_scope = scope;
> -		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
> -		fl4.flowi4_tun_key.tun_id = 0;
> +		struct flowi4 fl4 = {
> +			.flowi4_iif = LOOPBACK_IFINDEX,
> +			.daddr = ip_hdr(skb)->saddr,
> +			.flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
> +			.flowi4_scope = scope,
> +			.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
> +		};
>  		if (!fib_lookup(net, &fl4, &res, 0))
>  			return FIB_RES_PREFSRC(net, res);
>  	} else {
> --
> 2.5.5
> 
> 

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[David Miller]

From: Lance Richardson <lrichard@redhat.com>
Date: Tue, 22 Mar 2016 14:58:59 -0400 (EDT)

> Apologies, that should have been [PATCH v2 net].

No worries.

Applied and queued up for -stable, thanks.

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[David Ahern]

On 3/22/16 12:56 PM, Lance Richardson wrote:
> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
> before calling fib_lookup(), which means fib_table_lookup() is
> using non-deterministic data at this line:
>
> 	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
>
> Fix by initializing the entire fl4 structure, which will prevent
> similar issues as fields are added in the future by ensuring that
> all fields are initialized to zero unless explicitly initialized
> to another value.
>
> Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
> Suggested-by: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Lance Richardson <lrichard@redhat.com>
> ---
>   net/ipv4/fib_frontend.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 21add55..8a9246d 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -280,7 +280,6 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>   	struct in_device *in_dev;
>   	struct fib_result res;
>   	struct rtable *rt;
> -	struct flowi4 fl4;
>   	struct net *net;
>   	int scope;
>
> @@ -296,14 +295,13 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>
>   	scope = RT_SCOPE_UNIVERSE;
>   	if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
> -		fl4.flowi4_oif = 0;
> -		fl4.flowi4_iif = LOOPBACK_IFINDEX;
> -		fl4.daddr = ip_hdr(skb)->saddr;
> -		fl4.saddr = 0;
> -		fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
> -		fl4.flowi4_scope = scope;
> -		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
> -		fl4.flowi4_tun_key.tun_id = 0;
> +		struct flowi4 fl4 = {
> +			.flowi4_iif = LOOPBACK_IFINDEX,
> +			.daddr = ip_hdr(skb)->saddr,
> +			.flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
> +			.flowi4_scope = scope,
> +			.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
> +		};
>   		if (!fib_lookup(net, &fl4, &res, 0))
>   			return FIB_RES_PREFSRC(net, res);
>   	} else {
>

Acked-by: David Ahern <dsa@cumulusnetworks.com>

DaveM: this should go into stable releases back to v4.3.

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Eric Dumazet]

On Tue, 2016-03-22 at 13:00 -0600, David Ahern wrote:
> On 3/22/16 12:56 PM, Lance Richardson wrote:

> >
> > Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")

> DaveM: this should go into stable releases back to v4.3.
> 

The 'Fixes' tag tells this already ;)

$ git describe --contains 58189ca7b2741
v4.3-rc3~13^2~63

Re: [PATCH net] ipv4: initialize flowi4_flags before calling fib_lookup()

[Sergei Shtylyov]

Hello.

On 3/22/2016 9:56 PM, Lance Richardson wrote:

> Field fl4.flowi4_flags is not initialized in fib_compute_spec_dst()
> before calling fib_lookup(), which means fib_table_lookup() is
> using non-deterministic data at this line:
>
> 	if (!(flp->flowi4_flags & FLOWI_FLAG_SKIP_NH_OIF)) {
>
> Fix by initializing the entire fl4 structure, which will prevent
> similar issues as fields are added in the future by ensuring that
> all fields are initialized to zero unless explicitly initialized
> to another value.
>
> Fixes: 58189ca7b2741 ("net: Fix vti use case with oif in dst lookups")
> Suggested-by: David Ahern <dsa@cumulusnetworks.com>
> Signed-off-by: Lance Richardson <lrichard@redhat.com>
> ---
>   net/ipv4/fib_frontend.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 21add55..8a9246d 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
[...]
> @@ -296,14 +295,13 @@ __be32 fib_compute_spec_dst(struct sk_buff *skb)
>
>   	scope = RT_SCOPE_UNIVERSE;
>   	if (!ipv4_is_zeronet(ip_hdr(skb)->saddr)) {
> -		fl4.flowi4_oif = 0;
> -		fl4.flowi4_iif = LOOPBACK_IFINDEX;
> -		fl4.daddr = ip_hdr(skb)->saddr;
> -		fl4.saddr = 0;
> -		fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
> -		fl4.flowi4_scope = scope;
> -		fl4.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0;
> -		fl4.flowi4_tun_key.tun_id = 0;
> +		struct flowi4 fl4 = {
> +			.flowi4_iif = LOOPBACK_IFINDEX,
> +			.daddr = ip_hdr(skb)->saddr,
> +			.flowi4_tos = RT_TOS(ip_hdr(skb)->tos),
> +			.flowi4_scope = scope,
> +			.flowi4_mark = IN_DEV_SRC_VMARK(in_dev) ? skb->mark : 0,
> +		};

    Need empty line after the declaration.

>   		if (!fib_lookup(net, &fl4, &res, 0))
>   			return FIB_RES_PREFSRC(net, res);
>   	} else {

MBR, Sergei