Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[gregory hoggarth]

Hi,

I work for Allied Telesis, a company that manufactures and sells routers and switches.

Earlier in the year we upgraded the linux kernel on our L3 switch firmware to v3.6.2 which includes this patch. Our testers have picked up a change in behaviour which I have traced back to this patch and I'd like to get comments on whether this is a bug or not.

We found that in a configuration with a device under test (DUT) that has an IP address 192.168.0.1 / 24, sending an ICMP echo request with a source IP address of 192.168.0.255 will now be accepted by the DUT, which will send an ICMP echo reply back to the 192.168.0.255 address with a broadcast MAC address of FFFF.FFFF.FFFF, meaning any other devices in the L2 domain will pick up and process the ICMP reply.

Prior to the upgrade, our device was running on linux kernel 2.6.38.2, and in that version the DUT would reject the packet with a martian source address error:
11:20:54 awplus kernel: RxPkt(00001) Q:1 dev:8 lport:00000a00 cpuC:154 encap:0 len:98 bufs:1
11:20:54 awplus kernel: RxPkt(00001) vid:1 port2.0.23
11:20:54 awplus kernel: <7>   0000cd29 980d0000 cd27c1eb 08004500 
11:20:54 awplus kernel: <7>   00540000 40004001 b858c0a8 00ffc0a8 
11:20:54 awplus kernel: <7>   00010800 a4cf588c 000552dd c9990000 
11:20:54 awplus kernel: <7>   f3240809 0a0b0c0d 0e0f1011 12131415 
11:20:54 awplus kernel: <7>   (...)
11:20:54 awplus kernel: martian source 192.168.0.1 from 192.168.0.255, on dev vlan1
11:20:54 awplus kernel: ll header: 00:00:cd:29:98:0d:00:00:cd:27:c1:eb:08:00:45:00


Initially we were thinking that a device configured with 192.168.0.255 / 23 sending a ping to 192.168.0.1 / 24 should expect to get a reply, even though this is somewhat of a misconfiguration in that the subnets aren't matching and that some things may not work properly. For example ping from a device with IP address 192.168.0.254 would behave correctly, so it seemed correct that a ping from 192.168.0.255 should also work properly. However it appears that some other part of the kernel is detecting that the echo reply packet sent by the DUT has a destination IP address of the subnet broadcast, so instead of sending it with a unicast MAC address it sends it with the broadcast MAC address, which means all other devices on the subnet would process the ICMP echo reply. This therefore makes it s
 eem like the ICMP echo request should have been dropped in the first place.

The patch added the "if (!r && !fib_num_tclassid_users)" check to fib_validate_source, which for our devices evaluates to true, hence returning early and the source address check inside __fib_validate_source is not carried out.


Does anyone have comments as to what the correct behaviour should be in this case, and whether this is an unexpected side-effect from the patch or not?

Thanks,
Greg

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Julian Anastasov]

	Hello,

On Fri, 17 Jan 2014, gregory hoggarth wrote:

> Hi,
> 
> I work for Allied Telesis, a company that manufactures and sells routers and switches.
> 
> Earlier in the year we upgraded the linux kernel on our L3 switch firmware to v3.6.2 which includes this patch. Our testers have picked up a change in behaviour which I have traced back to this patch and I'd like to get comments on whether this is a bug or not.
> 
> We found that in a configuration with a device under test (DUT) that has an IP address 192.168.0.1 / 24, sending an ICMP echo request with a source IP address of 192.168.0.255 will now be accepted by the DUT, which will send an ICMP echo reply back to the 192.168.0.255 address with a broadcast MAC address of FFFF.FFFF.FFFF, meaning any other devices in the L2 domain will pick up and process the ICMP reply.

...

> Initially we were thinking that a device configured with 192.168.0.255 / 23 sending a ping to 192.168.0.1 / 24 should expect to get a reply, even though this is somewhat of a misconfiguration in that the subnets aren't matching and that some things may not work properly. For example ping from a device with IP address 192.168.0.254 would behave correctly, so it seemed correct that a ping from 192.168.0.255 should also work properly. However it appears that some other part of the kernel is detecting that the echo reply packet sent by the DUT has a destination IP address of the subnet broadcast, so instead of sending it with a unicast MAC address it sends it with the broadcast MAC address, which means all other devices on the subnet would process the ICMP echo reply. This therefore makes it
  seem like the ICMP echo request should have been dropped in the first place.
> 
> The patch added the "if (!r && !fib_num_tclassid_users)" check to fib_validate_source, which for our devices evaluates to true, hence returning early and the source address check inside __fib_validate_source is not carried out.
> 
> 
> Does anyone have comments as to what the correct behaviour should be in this case, and whether this is an unexpected side-effect from the patch or not?

	It seems only __fib_validate_source can reject all kind
of broadcast addresses in saddr. ip_route_input_slow() rejects
only the well known broadcasts. Without rp_filter may be we
can at least drop attempts to send replies back to broadcast
addresses? For example, checking result of ip_route_output_key() in
icmp_reply():

	if (rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
		=> ip_rt_put()

	For icmp_send() in icmp_route_lookup() ?

	For TCP in inet_csk_route_req() ?

	I did not checked all places...

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[gregory hoggarth]

Hi Julian,
 
>>> Julian Anastasov <ja@ssi.bg> 17/1/14 09:24 PM >>> 
> 
> 	Hello,
> 
> On Fri, 17 Jan 2014, gregory hoggarth wrote:
> 
>> Hi,
>> 
>> I work for Allied Telesis, a company that manufactures and sells routers and 
>> switches.
>> 
>> Earlier in the year we upgraded the linux kernel on our L3 switch firmware to 
>> v3.6.2 which includes this patch. Our testers have picked up a change in 
>> behaviour which I have traced back to this patch and I'd like to get comments 
>> on whether this is a bug or not.
>> 
>> We found that in a configuration with a device under test (DUT) that has an 
>> IP address 192.168.0.1 / 24, sending an ICMP echo request with a source IP 
>> address of 192.168.0.255 will now be accepted by the DUT, which will send an 
>> ICMP echo reply back to the 192.168.0.255 address with a broadcast MAC 
>> address of FFFF.FFFF.FFFF, meaning any other devices in the L2 domain will 
>> pick up and process the ICMP reply.
> 
> ...
> 
>> Initially we were thinking that a device configured with 192.168.0.255 / 23 
>> sending a ping to 192.168.0.1 / 24 should expect to get a reply, even 
>> though this is somewhat of a misconfiguration in that the subnets aren't 
>> matching and that some things may not work properly. For example ping from 
>> a device with IP address 192.168.0.254 would behave correctly, so it seemed 
>> correct that a ping from 192.168.0.255 should also work properly. However 
>> it appears that some other part of the kernel is detecting that the echo 
>> reply packet sent by the DUT has a destination IP address of the subnet 
>> broadcast, so instead of sending it with a unicast MAC address it sends it 
>> with the broadcast MAC address, which means all other devices on the subnet 
>> would process the ICMP echo reply. This therefore makes it seem like the 
>> ICMP echo request should have been dropped in the first place.
>>
>> The patch added the "if (!r && !fib_num_tclassid_users)" check to 
>> fib_validate_source, which for our devices evaluates to true, hence 
>> returning early and the source address check inside __fib_validate_source 
>> is not carried out.
>> 
>> 
>> Does anyone have comments as to what the correct behaviour should be in 
>> this case, and whether this is an unexpected side-effect from the patch or 
>> not?
>
>	It seems only __fib_validate_source can reject all kind
> of broadcast addresses in saddr. ip_route_input_slow() rejects
> only the well known broadcasts. Without rp_filter may be we
> can at least drop attempts to send replies back to broadcast
> addresses? For example, checking result of ip_route_output_key() in
> icmp_reply():
>
>	if (rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
>		=> ip_rt_put()
>
>	For icmp_send() in icmp_route_lookup() ?
>
>	For TCP in inet_csk_route_req() ?
>
>	I did not checked all places...
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>

Thank you for your reply.

While I think your solution may work, isn't the proper approach to drop these 
rogue packets, rather than wasting CPU and other resources processing them, 
and then beginning to craft responses which are in turn dropped?

Also seems better to drop the initial rogue packet, as that should cover all 
(?) different types of packets, rather than having to add small patches into 
ICMP, TCP and any other places that may need it.


So my question really is was the original patch correct and there's something
wrong with our device configuration, or is this an overlooked / untested side-
effect from that patch that as a result means the patch should be re-worked?


Thanks,
Greg

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Julian Anastasov]

	Hello,

On Mon, 20 Jan 2014, gregory hoggarth wrote:

> >>> Julian Anastasov <ja@ssi.bg> 17/1/14 09:24 PM >>> 
> >
> >	It seems only __fib_validate_source can reject all kind
> > of broadcast addresses in saddr. ip_route_input_slow() rejects
> > only the well known broadcasts. Without rp_filter may be we
> > can at least drop attempts to send replies back to broadcast
> > addresses? For example, checking result of ip_route_output_key() in
> > icmp_reply():
> >
> >	if (rt->rt_flags & (RTCF_BROADCAST | RTCF_MULTICAST))
> >		=> ip_rt_put()

> Thank you for your reply.
> 
> While I think your solution may work, isn't the proper approach to drop these 
> rogue packets, rather than wasting CPU and other resources processing them, 
> and then beginning to craft responses which are in turn dropped?

	The problem is that it is __fib_validate_source that
takes time for every packet. And it is a rare case to see
traffic from broadcast addresses. rp_filter set on external
interface will drop such packets. It is an option that one
can use even for internal interface, if needed.

> Also seems better to drop the initial rogue packet, as that should cover all 
> (?) different types of packets, rather than having to add small patches into 
> ICMP, TCP and any other places that may need it.

	Small check looks better compared to FIB lookup for
every received packet. Note that output route can be cached
for sockets, eg. TCP, so such small checks do not occur for
every reply packet.

> So my question really is was the original patch correct and there's something
> wrong with our device configuration, or is this an overlooked / untested side-
> effect from that patch that as a result means the patch should be re-worked?

	I don't remember someone mentioning about such
side-effect, I guess it is overlooked. IMHO, it is not a good
reason to restore the old behavior. Lets see other opinions.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Hannes Frederic Sowa]

On Wed, Jan 22, 2014 at 11:04:52PM +0200, Julian Anastasov wrote:
> 	I don't remember someone mentioning about such
> side-effect, I guess it is overlooked. IMHO, it is not a good
> reason to restore the old behavior. Lets see other opinions.

A routing lookup to check for broadcast is much too expansive, I agree. But
can't we just check skb->pkt_type == PACKET_BROADCAST?

Greetings,

  Hannes

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Hannes Frederic Sowa]

On Thu, Jan 23, 2014 at 03:02:24AM +0100, Hannes Frederic Sowa wrote:
> On Wed, Jan 22, 2014 at 11:04:52PM +0200, Julian Anastasov wrote:
> > 	I don't remember someone mentioning about such
> > side-effect, I guess it is overlooked. IMHO, it is not a good
> > reason to restore the old behavior. Lets see other opinions.
> 
> A routing lookup to check for broadcast is much too expansive, I agree. But
> can't we just check skb->pkt_type == PACKET_BROADCAST?

Ah, sorry, other way around, we need to check the source address. Maybe a
small hash table to look for currently installed broadcast addresses?

Greetings,

  Hannes

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Thu, 23 Jan 2014 03:02:24 +0100

> On Wed, Jan 22, 2014 at 11:04:52PM +0200, Julian Anastasov wrote:
>> 	I don't remember someone mentioning about such
>> side-effect, I guess it is overlooked. IMHO, it is not a good
>> reason to restore the old behavior. Lets see other opinions.
> 
> A routing lookup to check for broadcast is much too expansive, I agree. But
> can't we just check skb->pkt_type == PACKET_BROADCAST?

We can't universally know what a broadcast is, because the prefix of
every network is a local piece of information.

We might not have the route necessary to know the source address is
a broadcast if the packet came through more than 1 hop already.

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Hannes Frederic Sowa]

On Wed, Jan 22, 2014 at 06:53:05PM -0800, David Miller wrote:
> From: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Date: Thu, 23 Jan 2014 03:02:24 +0100
> 
> > On Wed, Jan 22, 2014 at 11:04:52PM +0200, Julian Anastasov wrote:
> >> 	I don't remember someone mentioning about such
> >> side-effect, I guess it is overlooked. IMHO, it is not a good
> >> reason to restore the old behavior. Lets see other opinions.
> > 
> > A routing lookup to check for broadcast is much too expansive, I agree. But
> > can't we just check skb->pkt_type == PACKET_BROADCAST?
> 
> We can't universally know what a broadcast is, because the prefix of
> every network is a local piece of information.
> 
> We might not have the route necessary to know the source address is
> a broadcast if the packet came through more than 1 hop already.

But in this case we only need those local pieces.

E.g. we register all local registered broadcast addresses in a structure
like inet_addr_lst so we only need to check if the packet would leave
this host with a broadcast hardware address. If the packet is forwarded
the router must do the same check as only it knows the local broadcast
addresses. I hope this is correct. ;)

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Julian Anastasov]

	Hello,

On Thu, 23 Jan 2014, Hannes Frederic Sowa wrote:

> E.g. we register all local registered broadcast addresses in a structure
> like inet_addr_lst so we only need to check if the packet would leave
> this host with a broadcast hardware address. If the packet is forwarded
> the router must do the same check as only it knows the local broadcast
> addresses. I hope this is correct. ;)

	Now when we can override the local table with ip
rules or to prepend route in local table, we can not be
sure that the broadcast routes are returned in all cases,
users can add unicast routes for such addresses.

	I don't remember for useful case where one may need to
override broadcast routes but adding such exceptions looks
risky.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: Linux kernel patch: elide fib_validate_source() completely when possible - bad side effect?

[Hannes Frederic Sowa]

Hi Julian,

On Thu, Jan 23, 2014 at 11:47:33AM +0200, Julian Anastasov wrote:
> On Thu, 23 Jan 2014, Hannes Frederic Sowa wrote:
> 
> > E.g. we register all local registered broadcast addresses in a structure
> > like inet_addr_lst so we only need to check if the packet would leave
> > this host with a broadcast hardware address. If the packet is forwarded
> > the router must do the same check as only it knows the local broadcast
> > addresses. I hope this is correct. ;)
> 
> 	Now when we can override the local table with ip
> rules or to prepend route in local table, we can not be
> sure that the broadcast routes are returned in all cases,
> users can add unicast routes for such addresses.
> 
> 	I don't remember for useful case where one may need to
> override broadcast routes but adding such exceptions looks
> risky.

The check I envisioned would only actually block the ifa_broadcast of only the
incoming interface as source address. So we wouldn't depend on the view of the
routing tables at all.

But I have no strong opinion on that.

Greetings,

  Hannes