[PATCH] net: neighbor: fix a crash caused by mod zero

[PATCH] net: neighbor: fix a crash caused by mod zero

[weichenchen]

pneigh_enqueue() tries to obtain a random delay by mod
NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
migth be zero at that point because someone could write zero
to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
callers check it.

This patch double-checks NEIGH_VAR(p, PROXY_DELAY) in
pneigh_enqueue() to ensure not to take zero as modulus.

Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
---
 net/core/neighbour.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 9500d28a43b0..eb5d015c53d3 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1570,9 +1570,14 @@ void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
 		    struct sk_buff *skb)
 {
 	unsigned long now = jiffies;
+	unsigned long sched_next;
 
-	unsigned long sched_next = now + (prandom_u32() %
-					  NEIGH_VAR(p, PROXY_DELAY));
+	int delay = NEIGH_VAR(p, PROXY_DELAY);
+
+	if (delay <= 0)
+		sched_next = now;
+	else
+		sched_next = now + (prandom_u32() % delay);
 
 	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
 		kfree_skb(skb);
-- 
2.20.1 (Apple Git-117)

Re: [PATCH] net: neighbor: fix a crash caused by mod zero

[Jakub Kicinski]

On Fri, 18 Dec 2020 12:20:19 +0800 weichenchen wrote:
> pneigh_enqueue() tries to obtain a random delay by mod
> NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
> migth be zero at that point because someone could write zero
> to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
> callers check it.
> 
> This patch double-checks NEIGH_VAR(p, PROXY_DELAY) in
> pneigh_enqueue() to ensure not to take zero as modulus.
> 
> Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>

Let's have the caller pass in the value since it did the checking?

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 9500d28a43b0..eb5d015c53d3 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1570,9 +1570,14 @@ void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
>  		    struct sk_buff *skb)
>  {
>  	unsigned long now = jiffies;
> +	unsigned long sched_next;
>  
> -	unsigned long sched_next = now + (prandom_u32() %
> -					  NEIGH_VAR(p, PROXY_DELAY));
> +	int delay = NEIGH_VAR(p, PROXY_DELAY);
> +
> +	if (delay <= 0)

Not that this still doesn't guarantee that the compiler won't re-read
the value (however unlikely). We need a READ_ONCE().

> +		sched_next = now;
> +	else
> +		sched_next = now + (prandom_u32() % delay);
>  
>  	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
>  		kfree_skb(skb);

[PATCH v2] net: neighbor: fix a crash caused by mod zero

[weichenchen]

pneigh_enqueue() tries to obtain a random delay by mod
NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
migth be zero at that point because someone could write zero
to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
callers check it.

This patch double-checks NEIGH_VAR(p, PROXY_DELAY) in
pneigh_enqueue() to ensure not to take zero as modulus.

Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
---
V2:
    - Use READ_ONCE() to prevent the complier from re-reading
      NEIGH_VAR(p, PROXY_DELAY).
    - Give a hint to the complier that delay <= 0 is unlikely
      to happen.

Note: I don't think having the caller pass in the value is a
good idea mainly because delay should be only decided by
/proc/sys/net/ipv4/neigh/[device]/proxy_delay rather than the
caller.
---
 net/core/neighbour.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 9500d28a43b0..7b03d3f129c0 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1570,9 +1570,14 @@ void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
 		    struct sk_buff *skb)
 {
 	unsigned long now = jiffies;
+	unsigned long sched_next;
 
-	unsigned long sched_next = now + (prandom_u32() %
-					  NEIGH_VAR(p, PROXY_DELAY));
+	int delay = READ_ONCE(NEIGH_VAR(p, PROXY_DELAY));
+
+	if (unlikely(delay <= 0))
+		sched_next = now;
+	else
+		sched_next = now + (prandom_u32() % delay);
 
 	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
 		kfree_skb(skb);
-- 
2.20.1 (Apple Git-117)

Re: [PATCH v2] net: neighbor: fix a crash caused by mod zero

[Jakub Kicinski]

On Mon, 21 Dec 2020 21:07:44 +0800 weichenchen wrote:
> pneigh_enqueue() tries to obtain a random delay by mod
> NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
> migth be zero at that point because someone could write zero
> to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
> callers check it.
> 
> This patch double-checks NEIGH_VAR(p, PROXY_DELAY) in
> pneigh_enqueue() to ensure not to take zero as modulus.
> 
> Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
> ---
> V2:
>     - Use READ_ONCE() to prevent the complier from re-reading
>       NEIGH_VAR(p, PROXY_DELAY).
>     - Give a hint to the complier that delay <= 0 is unlikely
>       to happen.
> 
> Note: I don't think having the caller pass in the value is a
> good idea mainly because delay should be only decided by
> /proc/sys/net/ipv4/neigh/[device]/proxy_delay rather than the
> caller.

In terms of not breaking abstraction? The decision to call 
this helper or not is made in the caller. And both callers 
do a NEIGH_VAR(p, PROXY_DELAY) == 0 check before making the 
call.

It seems like if the caller used READ_ONCE and passed the value 
in we would save ourselves the potentially surprising code flow.

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 9500d28a43b0..7b03d3f129c0 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1570,9 +1570,14 @@ void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
>  		    struct sk_buff *skb)
>  {
>  	unsigned long now = jiffies;
> +	unsigned long sched_next;
>  
> -	unsigned long sched_next = now + (prandom_u32() %
> -					  NEIGH_VAR(p, PROXY_DELAY));
> +	int delay = READ_ONCE(NEIGH_VAR(p, PROXY_DELAY));
> +
> +	if (unlikely(delay <= 0))
> +		sched_next = now;
> +	else
> +		sched_next = now + (prandom_u32() % delay);
>  
>  	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
>  		kfree_skb(skb);

[PATCH v3] net: neighbor: fix a crash caused by mod zero

[weichenchen]

pneigh_enqueue() tries to obtain a random delay by mod
NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
migth be zero at that point because someone could write zero
to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
callers check it.

This patch makes pneigh_enqueue() get a delay time passed in
by the callers and the callers guarantee it is not zero.

Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
---
V3:
    - Callers need to pass the delay time to pneigh_enqueue()
      now and they should guarantee it is not zero.
    - Use READ_ONCE() to read NEIGH_VAR(p, PROXY_DELAY) in both
      of the existing callers of pneigh_enqueue() and then pass
      it to pneigh_enqueue().
V2:
    - Use READ_ONCE() to prevent the complier from re-reading
      NEIGH_VAR(p, PROXY_DELAY).
    - Give a hint to the complier that delay <= 0 is unlikely
      to happen.
---
 include/net/neighbour.h | 2 +-
 net/core/neighbour.c    | 5 ++---
 net/ipv4/arp.c          | 8 +++++---
 net/ipv6/ndisc.c        | 6 +++---
 4 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 22ced1381ede..f7564dc5304d 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -352,7 +352,7 @@ struct net *neigh_parms_net(const struct neigh_parms *parms)
 unsigned long neigh_rand_reach_time(unsigned long base);
 
 void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
-		    struct sk_buff *skb);
+		    struct sk_buff *skb, int delay);
 struct pneigh_entry *pneigh_lookup(struct neigh_table *tbl, struct net *net,
 				   const void *key, struct net_device *dev,
 				   int creat);
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 9500d28a43b0..b440f966d109 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1567,12 +1567,11 @@ static void neigh_proxy_process(struct timer_list *t)
 }
 
 void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
-		    struct sk_buff *skb)
+		    struct sk_buff *skb, int delay)
 {
 	unsigned long now = jiffies;
 
-	unsigned long sched_next = now + (prandom_u32() %
-					  NEIGH_VAR(p, PROXY_DELAY));
+	unsigned long sched_next = now + (prandom_u32() % delay);
 
 	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
 		kfree_skb(skb);
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index 922dd73e5740..6ddce6e0a648 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -841,20 +841,22 @@ static int arp_process(struct net *net, struct sock *sk, struct sk_buff *skb)
 			     arp_fwd_pvlan(in_dev, dev, rt, sip, tip) ||
 			     (rt->dst.dev != dev &&
 			      pneigh_lookup(&arp_tbl, net, &tip, dev, 0)))) {
+				int delay;
+
 				n = neigh_event_ns(&arp_tbl, sha, &sip, dev);
 				if (n)
 					neigh_release(n);
 
+				delay = READ_ONCE(NEIGH_VAR(in_dev->arp_parms, PROXY_DELAY));
 				if (NEIGH_CB(skb)->flags & LOCALLY_ENQUEUED ||
-				    skb->pkt_type == PACKET_HOST ||
-				    NEIGH_VAR(in_dev->arp_parms, PROXY_DELAY) == 0) {
+				    skb->pkt_type == PACKET_HOST || delay == 0) {
 					arp_send_dst(ARPOP_REPLY, ETH_P_ARP,
 						     sip, dev, tip, sha,
 						     dev->dev_addr, sha,
 						     reply_dst);
 				} else {
 					pneigh_enqueue(&arp_tbl,
-						       in_dev->arp_parms, skb);
+						       in_dev->arp_parms, skb, delay);
 					goto out_free_dst;
 				}
 				goto out_consume_skb;
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 76717478f173..efdaaab47535 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -892,10 +892,10 @@ static void ndisc_recv_ns(struct sk_buff *skb)
 		    (idev->cnf.forwarding &&
 		     (net->ipv6.devconf_all->proxy_ndp || idev->cnf.proxy_ndp) &&
 		     (is_router = pndisc_is_router(&msg->target, dev)) >= 0)) {
+			int delay = READ_ONCE(NEIGH_VAR(idev->nd_parms, PROXY_DELAY));
 			if (!(NEIGH_CB(skb)->flags & LOCALLY_ENQUEUED) &&
 			    skb->pkt_type != PACKET_HOST &&
-			    inc &&
-			    NEIGH_VAR(idev->nd_parms, PROXY_DELAY) != 0) {
+			    inc && delay != 0) {
 				/*
 				 * for anycast or proxy,
 				 * sender should delay its response
@@ -905,7 +905,7 @@ static void ndisc_recv_ns(struct sk_buff *skb)
 				 */
 				struct sk_buff *n = skb_clone(skb, GFP_ATOMIC);
 				if (n)
-					pneigh_enqueue(&nd_tbl, idev->nd_parms, n);
+					pneigh_enqueue(&nd_tbl, idev->nd_parms, n, delay);
 				goto out;
 			}
 		} else
-- 
2.20.1 (Apple Git-117)

Re: [PATCH v3] net: neighbor: fix a crash caused by mod zero

[Eric Dumazet]

On 12/22/20 1:38 PM, weichenchen wrote:
> pneigh_enqueue() tries to obtain a random delay by mod
> NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
> migth be zero at that point because someone could write zero
> to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
> callers check it.
> 
> This patch makes pneigh_enqueue() get a delay time passed in
> by the callers and the callers guarantee it is not zero.
> 
> Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
> ---
> V3:
>     - Callers need to pass the delay time to pneigh_enqueue()
>       now and they should guarantee it is not zero.
>     - Use READ_ONCE() to read NEIGH_VAR(p, PROXY_DELAY) in both
>       of the existing callers of pneigh_enqueue() and then pass
>       it to pneigh_enqueue().
> V2:
>     - Use READ_ONCE() to prevent the complier from re-reading
>       NEIGH_VAR(p, PROXY_DELAY).
>     - Give a hint to the complier that delay <= 0 is unlikely
>       to happen.
> ---
>  include/net/neighbour.h | 2 +-
>  net/core/neighbour.c    | 5 ++---
>  net/ipv4/arp.c          | 8 +++++---
>  net/ipv6/ndisc.c        | 6 +++---
>  4 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 22ced1381ede..f7564dc5304d 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -352,7 +352,7 @@ struct net *neigh_parms_net(const struct neigh_parms *parms)
>  unsigned long neigh_rand_reach_time(unsigned long base);
>  
>  void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
> -		    struct sk_buff *skb);
> +		    struct sk_buff *skb, int delay);
>  struct pneigh_entry *pneigh_lookup(struct neigh_table *tbl, struct net *net,
>  				   const void *key, struct net_device *dev,
>  				   int creat);
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 9500d28a43b0..b440f966d109 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1567,12 +1567,11 @@ static void neigh_proxy_process(struct timer_list *t)
>  }
>  
>  void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
> -		    struct sk_buff *skb)
> +		    struct sk_buff *skb, int delay)
>  {
>  	unsigned long now = jiffies;
>  
> -	unsigned long sched_next = now + (prandom_u32() %
> -					  NEIGH_VAR(p, PROXY_DELAY));
> +	unsigned long sched_next = now + (prandom_u32() % delay);
>  
>  	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
>  		kfree_skb(skb);

This seems rather complex, what about not using a divide in the first place ? :

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 9500d28a43b0e1a390382912b6fb59db935e727b..745bc89acc87c2a4802fb6f301c11edd2f0096da 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1569,10 +1569,7 @@ static void neigh_proxy_process(struct timer_list *t)
 void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
                    struct sk_buff *skb)
 {
-       unsigned long now = jiffies;
-
-       unsigned long sched_next = now + (prandom_u32() %
-                                         NEIGH_VAR(p, PROXY_DELAY));
+       unsigned long sched_next = jiffies + prandom_u32_max(NEIGH_VAR(p, PROXY_DELAY));
 
        if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
                kfree_skb(skb);

[PATCH v4] net: neighbor: fix a crash caused by mod zero

[weichenchen]

pneigh_enqueue() tries to obtain a random delay by mod
NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
migth be zero at that point because someone could write zero
to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
callers check it.

This patch uses prandom_u32_max() to get a random delay instead
which avoids potential division by zero.

Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
---
V4:
    - Use prandom_u32_max() to get a random delay in
      pneigh_enqueue().
V3:
    - Callers need to pass the delay time to pneigh_enqueue()
      now and they should guarantee it is not zero.
    - Use READ_ONCE() to read NEIGH_VAR(p, PROXY_DELAY) in both
      of the existing callers of pneigh_enqueue() and then pass
      it to pneigh_enqueue().
V2:
    - Use READ_ONCE() to prevent the complier from re-reading
      NEIGH_VAR(p, PROXY_DELAY).
    - Give a hint to the complier that delay <= 0 is unlikely
      to happen.

V4 is quite concise and works well.
Thanks for Eric's and Jakub's advice.
---
 net/core/neighbour.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 9500d28a43b0..277ed854aef1 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1569,10 +1569,8 @@ static void neigh_proxy_process(struct timer_list *t)
 void pneigh_enqueue(struct neigh_table *tbl, struct neigh_parms *p,
 		    struct sk_buff *skb)
 {
-	unsigned long now = jiffies;
-
-	unsigned long sched_next = now + (prandom_u32() %
-					  NEIGH_VAR(p, PROXY_DELAY));
+	unsigned long sched_next = jiffies +
+			prandom_u32_max(NEIGH_VAR(p, PROXY_DELAY));
 
 	if (tbl->proxy_queue.qlen > NEIGH_VAR(p, PROXY_QLEN)) {
 		kfree_skb(skb);
-- 
2.20.1 (Apple Git-117)

Re: [PATCH v4] net: neighbor: fix a crash caused by mod zero

[David Miller]

From: weichenchen <weichen.chen@linux.alibaba.com>
Date: Fri, 25 Dec 2020 13:44:45 +0800

> pneigh_enqueue() tries to obtain a random delay by mod
> NEIGH_VAR(p, PROXY_DELAY). However, NEIGH_VAR(p, PROXY_DELAY)
> migth be zero at that point because someone could write zero
> to /proc/sys/net/ipv4/neigh/[device]/proxy_delay after the
> callers check it.
> 
> This patch uses prandom_u32_max() to get a random delay instead
> which avoids potential division by zero.
> 
> Signed-off-by: weichenchen <weichen.chen@linux.alibaba.com>
> ---
> V4:
>     - Use prandom_u32_max() to get a random delay in
>       pneigh_enqueue().
> V3:
>     - Callers need to pass the delay time to pneigh_enqueue()
>       now and they should guarantee it is not zero.
>     - Use READ_ONCE() to read NEIGH_VAR(p, PROXY_DELAY) in both
>       of the existing callers of pneigh_enqueue() and then pass
>       it to pneigh_enqueue().
> V2:
>     - Use READ_ONCE() to prevent the complier from re-reading
>       NEIGH_VAR(p, PROXY_DELAY).
>     - Give a hint to the complier that delay <= 0 is unlikely
>       to happen.
> 
> V4 is quite concise and works well.
> Thanks for Eric's and Jakub's advice.

Applied and queued up for -stable, thanks.