Re: [46/74] net: restore ip source validation

Re: [46/74] net: restore ip source validation

[Randolf Pohl]

[...]

> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -490,6 +490,7 @@ enum
>  	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
>  	NET_IPV4_CONF_ARP_ACCEPT=21,
>  	NET_IPV4_CONF_ARP_NOTIFY=22,
> +	NET_IPV4_CONF_SRC_VMARK=24,
>  	__NET_IPV4_CONF_MAX
>  };
> 

Must these be contiguous, i.e. 23?

R.
-- 
Jetzt kostenlos herunterladen: Internet Explorer 8 und Mozilla Firefox 3.5 -
sicherer, schneller und einfacher! http://portal.gmx.net/de/go/atbrowser

Re: [46/74] net: restore ip source validation

[jamal]

On Fri, 2010-02-05 at 15:34 +0100, Randolf Pohl wrote:
> [...]
> 
> > --- a/include/linux/sysctl.h
> > +++ b/include/linux/sysctl.h
> > @@ -490,6 +490,7 @@ enum
> >  	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
> >  	NET_IPV4_CONF_ARP_ACCEPT=21,
> >  	NET_IPV4_CONF_ARP_NOTIFY=22,
> > +	NET_IPV4_CONF_SRC_VMARK=24,
> >  	__NET_IPV4_CONF_MAX
> >  };
> > 
> 
> Must these be contiguous, i.e. 23?

The problem is elsewhere. Here's the fix.

--
diff --git a/kernel/sysctl_check.c b/kernel/sysctl_check.c
index b6e7aae..469193c 100644
--- a/kernel/sysctl_check.c
+++ b/kernel/sysctl_check.c
@@ -220,6 +220,7 @@ static const struct trans_ctl_table
trans_net_ipv4_conf_vars_table[] = {
        { NET_IPV4_CONF_PROMOTE_SECONDARIES,    "promote_secondaries" },
        { NET_IPV4_CONF_ARP_ACCEPT,             "arp_accept" },
        { NET_IPV4_CONF_ARP_NOTIFY,             "arp_notify" },
+       { NET_IPV4_CONF_SRC_VMARK,              "src_valid_mark" },
        {}
 };
---

cheers,
jamal

Re: [46/74] net: restore ip source validation

[Sven Joachim]

On 2010-02-05 15:51 +0100, jamal wrote:

> The problem is elsewhere. Here's the fix.
>
> --
> diff --git a/kernel/sysctl_check.c b/kernel/sysctl_check.c
> index b6e7aae..469193c 100644
> --- a/kernel/sysctl_check.c
> +++ b/kernel/sysctl_check.c
> @@ -220,6 +220,7 @@ static const struct trans_ctl_table
> trans_net_ipv4_conf_vars_table[] = {
>         { NET_IPV4_CONF_PROMOTE_SECONDARIES,    "promote_secondaries" },
>         { NET_IPV4_CONF_ARP_ACCEPT,             "arp_accept" },
>         { NET_IPV4_CONF_ARP_NOTIFY,             "arp_notify" },
> +       { NET_IPV4_CONF_SRC_VMARK,              "src_valid_mark" },
>         {}
>  };
> ---

Well spotted, that fixes it.  Your mailer is broken however, it
converted tabs into spaces, so I had to add the changed line manually.

Sven

Re: [46/74] net: restore ip source validation

[Eric W. Biederman]

Sven Joachim <svenjoac@gmx.de> writes:

> On 2010-02-05 15:51 +0100, jamal wrote:
>
>> The problem is elsewhere. Here's the fix.
>>
>> --
>> diff --git a/kernel/sysctl_check.c b/kernel/sysctl_check.c
>> index b6e7aae..469193c 100644
>> --- a/kernel/sysctl_check.c
>> +++ b/kernel/sysctl_check.c
>> @@ -220,6 +220,7 @@ static const struct trans_ctl_table
>> trans_net_ipv4_conf_vars_table[] = {
>>         { NET_IPV4_CONF_PROMOTE_SECONDARIES,    "promote_secondaries" },
>>         { NET_IPV4_CONF_ARP_ACCEPT,             "arp_accept" },
>>         { NET_IPV4_CONF_ARP_NOTIFY,             "arp_notify" },
>> +       { NET_IPV4_CONF_SRC_VMARK,              "src_valid_mark" },
>>         {}
>>  };
>> ---
>
> Well spotted, that fixes it.  Your mailer is broken however, it
> converted tabs into spaces, so I had to add the changed line manually.

Bah.  That DEVINET_SYSCTL_ENTRY requires having a binary sysctl
assigned, just to use as an index.

Which of course trips over all of the fine checks in sysctl_check.c to
keep people from assigning new binary sysctls by accident.

That is the only place in the kernel where he have that problem, I wonder
how much work it will be to finish untangling.

Eric

Re: [46/74] net: restore ip source validation

[Sven Joachim]

On 2010-02-05 21:17 +0100, Eric W. Biederman wrote:

> Bah.  That DEVINET_SYSCTL_ENTRY requires having a binary sysctl
> assigned, just to use as an index.
>
> Which of course trips over all of the fine checks in sysctl_check.c to
> keep people from assigning new binary sysctls by accident.
>
> That is the only place in the kernel where he have that problem, I wonder
> how much work it will be to finish untangling.

Isn't that already done in 2.6.33, looking at commit 83ac201b ?  Forgive
my ignorance, I am a layman.

Sen

Re: [46/74] net: restore ip source validation

[Eric W. Biederman]

Sven Joachim <svenjoac@gmx.de> writes:

> On 2010-02-05 21:17 +0100, Eric W. Biederman wrote:
>
>> Bah.  That DEVINET_SYSCTL_ENTRY requires having a binary sysctl
>> assigned, just to use as an index.
>>
>> Which of course trips over all of the fine checks in sysctl_check.c to
>> keep people from assigning new binary sysctls by accident.
>>
>> That is the only place in the kernel where he have that problem, I wonder
>> how much work it will be to finish untangling.
>
> Isn't that already done in 2.6.33, looking at commit 83ac201b ?  Forgive
> my ignorance, I am a layman.

In 2.6.33 the enumeration in sysctl.h still serves double duty as an index
into a per network device bitmap and as the binary sysctl number.  You are
correct that the rest of the binary sysctl code is decoupled in 2.6.33.

In 2.6.32 the implementation is also still coupled and that difference is
what caused problems for the backport.

I just wince whenever I noticed we have touched sysctl.h.

Eric

Re: [46/74] net: restore ip source validation

[Eric W. Biederman]

Sven Joachim <svenjoac@gmx.de> writes:

> On 2010-02-05 21:17 +0100, Eric W. Biederman wrote:
>
>> Bah.  That DEVINET_SYSCTL_ENTRY requires having a binary sysctl
>> assigned, just to use as an index.
>>
>> Which of course trips over all of the fine checks in sysctl_check.c to
>> keep people from assigning new binary sysctls by accident.
>>
>> That is the only place in the kernel where he have that problem, I wonder
>> how much work it will be to finish untangling.
>
> Isn't that already done in 2.6.33, looking at commit 83ac201b ?  Forgive
> my ignorance, I am a layman.

In 2.6.33 the enumeration in sysctl.h still serves double duty as an index
into a per network device bitmap and as the binary sysctl number.  You are
correct that the rest of the binary sysctl code is decoupled in 2.6.33.

In 2.6.32 the implementation is also still coupled and that difference is
what caused problems for the backport.

I just wince whenever I noticed we have touched sysctl.h.

Eric

Re: [46/74] net: restore ip source validation

[Sven Joachim]

On 2010-02-04 18:12 +0100, Greg KH wrote:

> 2.6.32-stable review patch.  If anyone has any objections, please let us know.

It's a bit hard to believe, but it is this patch which triggered the
boot-time crashes¹ that several people, including me, observed.
Reverting it avoids the kernel panic, and I'm running a kernel with the
other 73 patches applied right now.

Sven


¹ http://nelide.cz/downloads/2.6.32.8-crash.png

> ------------------
>
> From: Jamal Hadi Salim <hadi@cyberus.ca>
>
> [ Upstream commit 28f6aeea3f12d37bd258b2c0d5ba891bff4ec479 ]
>
> when using policy routing and the skb mark:
> there are cases where a back path validation requires us
> to use a different routing table for src ip validation than
> the one used for mapping ingress dst ip.
> One such a case is transparent proxying where we pretend to be
> the destination system and therefore the local table
> is used for incoming packets but possibly a main table would
> be used on outbound.
> Make the default behavior to allow the above and if users
> need to turn on the symmetry via sysctl src_valid_mark
>
> Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
> ---
>  include/linux/inetdevice.h |    1 +
>  include/linux/sysctl.h     |    1 +
>  net/ipv4/devinet.c         |    1 +
>  net/ipv4/fib_frontend.c    |    2 ++
>  4 files changed, 5 insertions(+)
>
> --- a/include/linux/inetdevice.h
> +++ b/include/linux/inetdevice.h
> @@ -83,6 +83,7 @@ static inline void ipv4_devconf_setall(s
>  #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
>  #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
>  #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
> +#define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
>  #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
>  						       ACCEPT_SOURCE_ROUTE)
>  #define IN_DEV_BOOTP_RELAY(in_dev)	IN_DEV_ANDCONF((in_dev), BOOTP_RELAY)
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -490,6 +490,7 @@ enum
>  	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
>  	NET_IPV4_CONF_ARP_ACCEPT=21,
>  	NET_IPV4_CONF_ARP_NOTIFY=22,
> +	NET_IPV4_CONF_SRC_VMARK=24,
>  	__NET_IPV4_CONF_MAX
>  };
>  
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -1450,6 +1450,7 @@ static struct devinet_sysctl_table {
>  		DEVINET_SYSCTL_RW_ENTRY(SEND_REDIRECTS, "send_redirects"),
>  		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_SOURCE_ROUTE,
>  					"accept_source_route"),
> +		DEVINET_SYSCTL_RW_ENTRY(SRC_VMARK, "src_valid_mark"),
>  		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP, "proxy_arp"),
>  		DEVINET_SYSCTL_RW_ENTRY(MEDIUM_ID, "medium_id"),
>  		DEVINET_SYSCTL_RW_ENTRY(BOOTP_RELAY, "bootp_relay"),
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -251,6 +251,8 @@ int fib_validate_source(__be32 src, __be
>  	if (in_dev) {
>  		no_addr = in_dev->ifa_list == NULL;
>  		rpf = IN_DEV_RPFILTER(in_dev);
> +		if (mark && !IN_DEV_SRC_VMARK(in_dev))
> +			fl.mark = 0;
>  	}
>  	rcu_read_unlock();

[46/74] net: restore ip source validation

[Greg KH]

2.6.32-stable review patch.  If anyone has any objections, please let us know.

------------------

From: Jamal Hadi Salim <hadi@cyberus.ca>

[ Upstream commit 28f6aeea3f12d37bd258b2c0d5ba891bff4ec479 ]

when using policy routing and the skb mark:
there are cases where a back path validation requires us
to use a different routing table for src ip validation than
the one used for mapping ingress dst ip.
One such a case is transparent proxying where we pretend to be
the destination system and therefore the local table
is used for incoming packets but possibly a main table would
be used on outbound.
Make the default behavior to allow the above and if users
need to turn on the symmetry via sysctl src_valid_mark

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

---
 include/linux/inetdevice.h |    1 +
 include/linux/sysctl.h     |    1 +
 net/ipv4/devinet.c         |    1 +
 net/ipv4/fib_frontend.c    |    2 ++
 4 files changed, 5 insertions(+)

--- a/include/linux/inetdevice.h
+++ b/include/linux/inetdevice.h
@@ -83,6 +83,7 @@ static inline void ipv4_devconf_setall(s
 #define IN_DEV_FORWARD(in_dev)		IN_DEV_CONF_GET((in_dev), FORWARDING)
 #define IN_DEV_MFORWARD(in_dev)		IN_DEV_ANDCONF((in_dev), MC_FORWARDING)
 #define IN_DEV_RPFILTER(in_dev)		IN_DEV_MAXCONF((in_dev), RP_FILTER)
+#define IN_DEV_SRC_VMARK(in_dev)    	IN_DEV_ORCONF((in_dev), SRC_VMARK)
 #define IN_DEV_SOURCE_ROUTE(in_dev)	IN_DEV_ANDCONF((in_dev), \
 						       ACCEPT_SOURCE_ROUTE)
 #define IN_DEV_BOOTP_RELAY(in_dev)	IN_DEV_ANDCONF((in_dev), BOOTP_RELAY)
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -490,6 +490,7 @@ enum
 	NET_IPV4_CONF_PROMOTE_SECONDARIES=20,
 	NET_IPV4_CONF_ARP_ACCEPT=21,
 	NET_IPV4_CONF_ARP_NOTIFY=22,
+	NET_IPV4_CONF_SRC_VMARK=24,
 	__NET_IPV4_CONF_MAX
 };
 
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1450,6 +1450,7 @@ static struct devinet_sysctl_table {
 		DEVINET_SYSCTL_RW_ENTRY(SEND_REDIRECTS, "send_redirects"),
 		DEVINET_SYSCTL_RW_ENTRY(ACCEPT_SOURCE_ROUTE,
 					"accept_source_route"),
+		DEVINET_SYSCTL_RW_ENTRY(SRC_VMARK, "src_valid_mark"),
 		DEVINET_SYSCTL_RW_ENTRY(PROXY_ARP, "proxy_arp"),
 		DEVINET_SYSCTL_RW_ENTRY(MEDIUM_ID, "medium_id"),
 		DEVINET_SYSCTL_RW_ENTRY(BOOTP_RELAY, "bootp_relay"),
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -251,6 +251,8 @@ int fib_validate_source(__be32 src, __be
 	if (in_dev) {
 		no_addr = in_dev->ifa_list == NULL;
 		rpf = IN_DEV_RPFILTER(in_dev);
+		if (mark && !IN_DEV_SRC_VMARK(in_dev))
+			fl.mark = 0;
 	}
 	rcu_read_unlock();