netdev - Re: [PATCH net-next] mpls: Infer payload of packet from via address family.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sidba1ea.fsf@x220.int.ebiederm.org>
Date:	Wed, 11 Mar 2015 12:29:17 -0500
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Robert Shearman <rshearma@...cade.com>
Cc:	<davem@...emloft.net>, <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] mpls: Infer payload of packet from via address family.

Robert Shearman <rshearma@...cade.com> writes:

> This ensures that if a routing protocol incorrectly advertises a label
> for a prefix whose address-family is inconsistent with that of the
> nexthop, then the traffic will be dropped, rather than the issue being
> silently worked around.

The address family of the next hop need have no particular relationship
to the address families you can send to the next hop.  As such I
consider the behavior your are proposing here actively wrong.  It
appears to block valid use cases such as using a single mpls label
to carry both ipv4 and ipv6 traffic simply because we use an ipv4 next
hop.

I am not opposed to adding configurability to force the issue,
especially as unexpected traffic may be a problem but I don't think that
should be the default.  I think the default for a tunnel egress should
be assume everyone sticking packets in that tunnel are playing nice so
we should decode as much as possible.

> The accessible skb length should also be validated prior to the IPv4
> or IPv6 headers being accessed, since only the label header will have
> previously been validated.

I agree I goofed by not including the appropriate pskb_may_pull checks.
And that needs to be fixed.

> Rename mpls_egress to mpls_egress_to_ip to make it more obvious that
> the function is used for traffic going out as IP, not for labeled
> traffic (or for the not-yet-implemented pseudo-wires).

I disagree.

The name of the function needs to be mpls_egress, and it should be
eventually expanded to handle as many cases are reasonable.  With the
default being to start the decode of packets by looking at the first
nibble.

Without explicit configuration it seems entirely reasonable to assume
that if the first nibble is 4 it is an ipv4 packet.  If the first nibble
is 6 it is an ipv6 packet.  If the first nibble is 1 it is a generic
association channel.  If the first nibble is 0 it has a control word and
it is a pseudo wire where the output tunnel type matches the output
device.

A handful of pseudo wires do things differently.  SONET sets bits in the
first nibble, Ethernet has cases where it does not include the control
word and as such the first nible might not be zero.   And then we have
oddball cases that need configuration such as should the ethernet
control words sequence number be honored.

I admit that supporting ethernet and similiar pseudo wires will require
the arguments to mpls_egress to be changed a little so that we can skip
taking the next hop address link layer address from the mpls_route, but
that does not mean we should just through it under a bus.

Fundamentally mpls_egress is the function that we call when the bottom
of stack indicator is reached.  It should either figure out that the
packet can be forwarded or it should indicate that the packet should be
dropped.

Eric
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html