* [Bridge] [PATCH net 0/2] Fix infinite loop in bridge and vxlan modules
@ 2020-06-01 12:58 ` Ido Schimmel
0 siblings, 0 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-06-01 12:58 UTC (permalink / raw)
To: netdev, bridge
Cc: mlxsw, nikolay, roopa, dlstevens, Ido Schimmel, allas, kuba, davem
From: Ido Schimmel <idosch@mellanox.com>
When suppressing invalid IPv6 Neighbour Solicitation messages, it is
possible for the bridge and vxlan modules to get stuck in an infinite
loop. See the individual changelogs for detailed explanation of the
problem and solution.
The bug was originally reported against the bridge module, but after
auditing the code base I found that the buggy code was copied from the
vxlan module. This patch set fixes both modules. Could not find more
instances of the problem.
Please consider both patches for stable releases.
Ido Schimmel (2):
bridge: Avoid infinite loop when suppressing NS messages with invalid
options
vxlan: Avoid infinite loop when suppressing NS messages with invalid
options
drivers/net/vxlan.c | 4 ++++
net/bridge/br_arp_nd_proxy.c | 4 ++++
2 files changed, 8 insertions(+)
--
2.26.2
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH net 1/2] bridge: Avoid infinite loop when suppressing NS messages with invalid options
2020-06-01 12:58 ` [Bridge] " Ido Schimmel
@ 2020-06-01 12:58 ` Ido Schimmel
-1 siblings, 0 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-06-01 12:58 UTC (permalink / raw)
To: netdev, bridge
Cc: davem, kuba, roopa, nikolay, dlstevens, allas, mlxsw, Ido Schimmel
From: Ido Schimmel <idosch@mellanox.com>
When neighbor suppression is enabled the bridge device might reply to
Neighbor Solicitation (NS) messages on behalf of remote hosts.
In case the NS message includes the "Source link-layer address" option
[1], the bridge device will use the specified address as the link-layer
destination address in its reply.
To avoid an infinite loop, break out of the options parsing loop when
encountering an option with length zero and disregard the NS message.
This is consistent with the IPv6 ndisc code and RFC 4886 which states
that "Nodes MUST silently discard an ND packet that contains an option
with length zero" [2].
[1] https://tools.ietf.org/html/rfc4861#section-4.3
[2] https://tools.ietf.org/html/rfc4861#section-4.6
Fixes: ed842faeb2bd ("bridge: suppress nd pkts on BR_NEIGH_SUPPRESS ports")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Alla Segal <allas@mellanox.com>
Tested-by: Alla Segal <allas@mellanox.com>
---
net/bridge/br_arp_nd_proxy.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 37908561a64b..b18cdf03edb3 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -276,6 +276,10 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
ns_olen = request->len - (skb_network_offset(request) +
sizeof(struct ipv6hdr)) - sizeof(*ns);
for (i = 0; i < ns_olen - 1; i += (ns->opt[i + 1] << 3)) {
+ if (!ns->opt[i + 1]) {
+ kfree_skb(reply);
+ return;
+ }
if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
break;
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Bridge] [PATCH net 1/2] bridge: Avoid infinite loop when suppressing NS messages with invalid options
@ 2020-06-01 12:58 ` Ido Schimmel
0 siblings, 0 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-06-01 12:58 UTC (permalink / raw)
To: netdev, bridge
Cc: mlxsw, nikolay, roopa, dlstevens, Ido Schimmel, allas, kuba, davem
From: Ido Schimmel <idosch@mellanox.com>
When neighbor suppression is enabled the bridge device might reply to
Neighbor Solicitation (NS) messages on behalf of remote hosts.
In case the NS message includes the "Source link-layer address" option
[1], the bridge device will use the specified address as the link-layer
destination address in its reply.
To avoid an infinite loop, break out of the options parsing loop when
encountering an option with length zero and disregard the NS message.
This is consistent with the IPv6 ndisc code and RFC 4886 which states
that "Nodes MUST silently discard an ND packet that contains an option
with length zero" [2].
[1] https://tools.ietf.org/html/rfc4861#section-4.3
[2] https://tools.ietf.org/html/rfc4861#section-4.6
Fixes: ed842faeb2bd ("bridge: suppress nd pkts on BR_NEIGH_SUPPRESS ports")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Alla Segal <allas@mellanox.com>
Tested-by: Alla Segal <allas@mellanox.com>
---
net/bridge/br_arp_nd_proxy.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
index 37908561a64b..b18cdf03edb3 100644
--- a/net/bridge/br_arp_nd_proxy.c
+++ b/net/bridge/br_arp_nd_proxy.c
@@ -276,6 +276,10 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
ns_olen = request->len - (skb_network_offset(request) +
sizeof(struct ipv6hdr)) - sizeof(*ns);
for (i = 0; i < ns_olen - 1; i += (ns->opt[i + 1] << 3)) {
+ if (!ns->opt[i + 1]) {
+ kfree_skb(reply);
+ return;
+ }
if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
break;
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net 1/2] bridge: Avoid infinite loop when suppressing NS messages with invalid options
2020-06-01 12:58 ` [Bridge] " Ido Schimmel
@ 2020-06-01 13:02 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 12+ messages in thread
From: Nikolay Aleksandrov @ 2020-06-01 13:02 UTC (permalink / raw)
To: Ido Schimmel, netdev, bridge
Cc: davem, kuba, roopa, dlstevens, allas, mlxsw, Ido Schimmel
On 01/06/2020 15:58, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@mellanox.com>
>
> When neighbor suppression is enabled the bridge device might reply to
> Neighbor Solicitation (NS) messages on behalf of remote hosts.
>
> In case the NS message includes the "Source link-layer address" option
> [1], the bridge device will use the specified address as the link-layer
> destination address in its reply.
>
> To avoid an infinite loop, break out of the options parsing loop when
> encountering an option with length zero and disregard the NS message.
>
> This is consistent with the IPv6 ndisc code and RFC 4886 which states
> that "Nodes MUST silently discard an ND packet that contains an option
> with length zero" [2].
>
> [1] https://tools.ietf.org/html/rfc4861#section-4.3
> [2] https://tools.ietf.org/html/rfc4861#section-4.6
>
> Fixes: ed842faeb2bd ("bridge: suppress nd pkts on BR_NEIGH_SUPPRESS ports")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Alla Segal <allas@mellanox.com>
> Tested-by: Alla Segal <allas@mellanox.com>
> ---
> net/bridge/br_arp_nd_proxy.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> index 37908561a64b..b18cdf03edb3 100644
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c
> @@ -276,6 +276,10 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
> ns_olen = request->len - (skb_network_offset(request) +
> sizeof(struct ipv6hdr)) - sizeof(*ns);
> for (i = 0; i < ns_olen - 1; i += (ns->opt[i + 1] << 3)) {
> + if (!ns->opt[i + 1]) {
> + kfree_skb(reply);
> + return;
> + }
> if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
> daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
> break;
>
Good catch!
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Bridge] [PATCH net 1/2] bridge: Avoid infinite loop when suppressing NS messages with invalid options
@ 2020-06-01 13:02 ` Nikolay Aleksandrov
0 siblings, 0 replies; 12+ messages in thread
From: Nikolay Aleksandrov @ 2020-06-01 13:02 UTC (permalink / raw)
To: Ido Schimmel, netdev, bridge
Cc: mlxsw, roopa, dlstevens, Ido Schimmel, allas, kuba, davem
On 01/06/2020 15:58, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@mellanox.com>
>
> When neighbor suppression is enabled the bridge device might reply to
> Neighbor Solicitation (NS) messages on behalf of remote hosts.
>
> In case the NS message includes the "Source link-layer address" option
> [1], the bridge device will use the specified address as the link-layer
> destination address in its reply.
>
> To avoid an infinite loop, break out of the options parsing loop when
> encountering an option with length zero and disregard the NS message.
>
> This is consistent with the IPv6 ndisc code and RFC 4886 which states
> that "Nodes MUST silently discard an ND packet that contains an option
> with length zero" [2].
>
> [1] https://tools.ietf.org/html/rfc4861#section-4.3
> [2] https://tools.ietf.org/html/rfc4861#section-4.6
>
> Fixes: ed842faeb2bd ("bridge: suppress nd pkts on BR_NEIGH_SUPPRESS ports")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Alla Segal <allas@mellanox.com>
> Tested-by: Alla Segal <allas@mellanox.com>
> ---
> net/bridge/br_arp_nd_proxy.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/bridge/br_arp_nd_proxy.c b/net/bridge/br_arp_nd_proxy.c
> index 37908561a64b..b18cdf03edb3 100644
> --- a/net/bridge/br_arp_nd_proxy.c
> +++ b/net/bridge/br_arp_nd_proxy.c
> @@ -276,6 +276,10 @@ static void br_nd_send(struct net_bridge *br, struct net_bridge_port *p,
> ns_olen = request->len - (skb_network_offset(request) +
> sizeof(struct ipv6hdr)) - sizeof(*ns);
> for (i = 0; i < ns_olen - 1; i += (ns->opt[i + 1] << 3)) {
> + if (!ns->opt[i + 1]) {
> + kfree_skb(reply);
> + return;
> + }
> if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
> daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
> break;
>
Good catch!
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net 2/2] vxlan: Avoid infinite loop when suppressing NS messages with invalid options
2020-06-01 12:58 ` [Bridge] " Ido Schimmel
@ 2020-06-01 12:58 ` Ido Schimmel
-1 siblings, 0 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-06-01 12:58 UTC (permalink / raw)
To: netdev, bridge
Cc: davem, kuba, roopa, nikolay, dlstevens, allas, mlxsw, Ido Schimmel
From: Ido Schimmel <idosch@mellanox.com>
When proxy mode is enabled the vxlan device might reply to Neighbor
Solicitation (NS) messages on behalf of remote hosts.
In case the NS message includes the "Source link-layer address" option
[1], the vxlan device will use the specified address as the link-layer
destination address in its reply.
To avoid an infinite loop, break out of the options parsing loop when
encountering an option with length zero and disregard the NS message.
This is consistent with the IPv6 ndisc code and RFC 4886 which states
that "Nodes MUST silently discard an ND packet that contains an option
with length zero" [2].
[1] https://tools.ietf.org/html/rfc4861#section-4.3
[2] https://tools.ietf.org/html/rfc4861#section-4.6
Fixes: 4b29dba9c085 ("vxlan: fix nonfunctional neigh_reduce()")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/vxlan.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index a5b415fed11e..779e56c43d27 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1924,6 +1924,10 @@ static struct sk_buff *vxlan_na_create(struct sk_buff *request,
ns_olen = request->len - skb_network_offset(request) -
sizeof(struct ipv6hdr) - sizeof(*ns);
for (i = 0; i < ns_olen-1; i += (ns->opt[i+1]<<3)) {
+ if (!ns->opt[i + 1]) {
+ kfree_skb(reply);
+ return NULL;
+ }
if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
break;
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [Bridge] [PATCH net 2/2] vxlan: Avoid infinite loop when suppressing NS messages with invalid options
@ 2020-06-01 12:58 ` Ido Schimmel
0 siblings, 0 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-06-01 12:58 UTC (permalink / raw)
To: netdev, bridge
Cc: mlxsw, nikolay, roopa, dlstevens, Ido Schimmel, allas, kuba, davem
From: Ido Schimmel <idosch@mellanox.com>
When proxy mode is enabled the vxlan device might reply to Neighbor
Solicitation (NS) messages on behalf of remote hosts.
In case the NS message includes the "Source link-layer address" option
[1], the vxlan device will use the specified address as the link-layer
destination address in its reply.
To avoid an infinite loop, break out of the options parsing loop when
encountering an option with length zero and disregard the NS message.
This is consistent with the IPv6 ndisc code and RFC 4886 which states
that "Nodes MUST silently discard an ND packet that contains an option
with length zero" [2].
[1] https://tools.ietf.org/html/rfc4861#section-4.3
[2] https://tools.ietf.org/html/rfc4861#section-4.6
Fixes: 4b29dba9c085 ("vxlan: fix nonfunctional neigh_reduce()")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/vxlan.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
index a5b415fed11e..779e56c43d27 100644
--- a/drivers/net/vxlan.c
+++ b/drivers/net/vxlan.c
@@ -1924,6 +1924,10 @@ static struct sk_buff *vxlan_na_create(struct sk_buff *request,
ns_olen = request->len - skb_network_offset(request) -
sizeof(struct ipv6hdr) - sizeof(*ns);
for (i = 0; i < ns_olen-1; i += (ns->opt[i+1]<<3)) {
+ if (!ns->opt[i + 1]) {
+ kfree_skb(reply);
+ return NULL;
+ }
if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
break;
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH net 2/2] vxlan: Avoid infinite loop when suppressing NS messages with invalid options
2020-06-01 12:58 ` [Bridge] " Ido Schimmel
@ 2020-06-01 13:02 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 12+ messages in thread
From: Nikolay Aleksandrov @ 2020-06-01 13:02 UTC (permalink / raw)
To: Ido Schimmel, netdev, bridge
Cc: davem, kuba, roopa, dlstevens, allas, mlxsw, Ido Schimmel
On 01/06/2020 15:58, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@mellanox.com>
>
> When proxy mode is enabled the vxlan device might reply to Neighbor
> Solicitation (NS) messages on behalf of remote hosts.
>
> In case the NS message includes the "Source link-layer address" option
> [1], the vxlan device will use the specified address as the link-layer
> destination address in its reply.
>
> To avoid an infinite loop, break out of the options parsing loop when
> encountering an option with length zero and disregard the NS message.
>
> This is consistent with the IPv6 ndisc code and RFC 4886 which states
> that "Nodes MUST silently discard an ND packet that contains an option
> with length zero" [2].
>
> [1] https://tools.ietf.org/html/rfc4861#section-4.3
> [2] https://tools.ietf.org/html/rfc4861#section-4.6
>
> Fixes: 4b29dba9c085 ("vxlan: fix nonfunctional neigh_reduce()")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
> drivers/net/vxlan.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index a5b415fed11e..779e56c43d27 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1924,6 +1924,10 @@ static struct sk_buff *vxlan_na_create(struct sk_buff *request,
> ns_olen = request->len - skb_network_offset(request) -
> sizeof(struct ipv6hdr) - sizeof(*ns);
> for (i = 0; i < ns_olen-1; i += (ns->opt[i+1]<<3)) {
> + if (!ns->opt[i + 1]) {
> + kfree_skb(reply);
> + return NULL;
> + }
> if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
> daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
> break;
>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Bridge] [PATCH net 2/2] vxlan: Avoid infinite loop when suppressing NS messages with invalid options
@ 2020-06-01 13:02 ` Nikolay Aleksandrov
0 siblings, 0 replies; 12+ messages in thread
From: Nikolay Aleksandrov @ 2020-06-01 13:02 UTC (permalink / raw)
To: Ido Schimmel, netdev, bridge
Cc: mlxsw, roopa, dlstevens, Ido Schimmel, allas, kuba, davem
On 01/06/2020 15:58, Ido Schimmel wrote:
> From: Ido Schimmel <idosch@mellanox.com>
>
> When proxy mode is enabled the vxlan device might reply to Neighbor
> Solicitation (NS) messages on behalf of remote hosts.
>
> In case the NS message includes the "Source link-layer address" option
> [1], the vxlan device will use the specified address as the link-layer
> destination address in its reply.
>
> To avoid an infinite loop, break out of the options parsing loop when
> encountering an option with length zero and disregard the NS message.
>
> This is consistent with the IPv6 ndisc code and RFC 4886 which states
> that "Nodes MUST silently discard an ND packet that contains an option
> with length zero" [2].
>
> [1] https://tools.ietf.org/html/rfc4861#section-4.3
> [2] https://tools.ietf.org/html/rfc4861#section-4.6
>
> Fixes: 4b29dba9c085 ("vxlan: fix nonfunctional neigh_reduce()")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> ---
> drivers/net/vxlan.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index a5b415fed11e..779e56c43d27 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1924,6 +1924,10 @@ static struct sk_buff *vxlan_na_create(struct sk_buff *request,
> ns_olen = request->len - skb_network_offset(request) -
> sizeof(struct ipv6hdr) - sizeof(*ns);
> for (i = 0; i < ns_olen-1; i += (ns->opt[i+1]<<3)) {
> + if (!ns->opt[i + 1]) {
> + kfree_skb(reply);
> + return NULL;
> + }
> if (ns->opt[i] == ND_OPT_SOURCE_LL_ADDR) {
> daddr = ns->opt + i + sizeof(struct nd_opt_hdr);
> break;
>
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 0/2] Fix infinite loop in bridge and vxlan modules
2020-06-01 12:58 ` [Bridge] " Ido Schimmel
@ 2020-06-01 18:09 ` David Miller
-1 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2020-06-01 18:09 UTC (permalink / raw)
To: idosch
Cc: netdev, bridge, kuba, roopa, nikolay, dlstevens, allas, mlxsw, idosch
From: Ido Schimmel <idosch@idosch.org>
Date: Mon, 1 Jun 2020 15:58:53 +0300
> From: Ido Schimmel <idosch@mellanox.com>
>
> When suppressing invalid IPv6 Neighbour Solicitation messages, it is
> possible for the bridge and vxlan modules to get stuck in an infinite
> loop. See the individual changelogs for detailed explanation of the
> problem and solution.
>
> The bug was originally reported against the bridge module, but after
> auditing the code base I found that the buggy code was copied from the
> vxlan module. This patch set fixes both modules. Could not find more
> instances of the problem.
>
> Please consider both patches for stable releases.
Series applied and queued up for -stable, thank you.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Bridge] [PATCH net 0/2] Fix infinite loop in bridge and vxlan modules
@ 2020-06-01 18:09 ` David Miller
0 siblings, 0 replies; 12+ messages in thread
From: David Miller @ 2020-06-01 18:09 UTC (permalink / raw)
To: idosch
Cc: mlxsw, nikolay, netdev, roopa, bridge, dlstevens, idosch, allas, kuba
From: Ido Schimmel <idosch@idosch.org>
Date: Mon, 1 Jun 2020 15:58:53 +0300
> From: Ido Schimmel <idosch@mellanox.com>
>
> When suppressing invalid IPv6 Neighbour Solicitation messages, it is
> possible for the bridge and vxlan modules to get stuck in an infinite
> loop. See the individual changelogs for detailed explanation of the
> problem and solution.
>
> The bug was originally reported against the bridge module, but after
> auditing the code base I found that the buggy code was copied from the
> vxlan module. This patch set fixes both modules. Could not find more
> instances of the problem.
>
> Please consider both patches for stable releases.
Series applied and queued up for -stable, thank you.
^ permalink raw reply [flat|nested] 12+ messages in thread