* [PATCH net v3] net: neigh: fix multiple neigh timer scheduling
@ 2019-07-14 21:36 Lorenzo Bianconi
2019-07-15 16:32 ` David Ahern
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2019-07-14 21:36 UTC (permalink / raw)
To: davem; +Cc: netdev, dsahern, marek
Neigh timer can be scheduled multiple times from userspace adding
multiple neigh entries and forcing the neigh timer scheduling passing
NTF_USE in the netlink requests.
This will result in a refcount leak and in the following dump stack:
[ 32.465295] NEIGH: BUG, double timer add, state is 8
[ 32.465308] CPU: 0 PID: 416 Comm: double_timer_ad Not tainted 5.2.0+ #65
[ 32.465311] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.12.0-2.fc30 04/01/2014
[ 32.465313] Call Trace:
[ 32.465318] dump_stack+0x7c/0xc0
[ 32.465323] __neigh_event_send+0x20c/0x880
[ 32.465326] ? ___neigh_create+0x846/0xfb0
[ 32.465329] ? neigh_lookup+0x2a9/0x410
[ 32.465332] ? neightbl_fill_info.constprop.0+0x800/0x800
[ 32.465334] neigh_add+0x4f8/0x5e0
[ 32.465337] ? neigh_xmit+0x620/0x620
[ 32.465341] ? find_held_lock+0x85/0xa0
[ 32.465345] rtnetlink_rcv_msg+0x204/0x570
[ 32.465348] ? rtnl_dellink+0x450/0x450
[ 32.465351] ? mark_held_locks+0x90/0x90
[ 32.465354] ? match_held_lock+0x1b/0x230
[ 32.465357] netlink_rcv_skb+0xc4/0x1d0
[ 32.465360] ? rtnl_dellink+0x450/0x450
[ 32.465363] ? netlink_ack+0x420/0x420
[ 32.465366] ? netlink_deliver_tap+0x115/0x560
[ 32.465369] ? __alloc_skb+0xc9/0x2f0
[ 32.465372] netlink_unicast+0x270/0x330
[ 32.465375] ? netlink_attachskb+0x2f0/0x2f0
[ 32.465378] netlink_sendmsg+0x34f/0x5a0
[ 32.465381] ? netlink_unicast+0x330/0x330
[ 32.465385] ? move_addr_to_kernel.part.0+0x20/0x20
[ 32.465388] ? netlink_unicast+0x330/0x330
[ 32.465391] sock_sendmsg+0x91/0xa0
[ 32.465394] ___sys_sendmsg+0x407/0x480
[ 32.465397] ? copy_msghdr_from_user+0x200/0x200
[ 32.465401] ? _raw_spin_unlock_irqrestore+0x37/0x40
[ 32.465404] ? lockdep_hardirqs_on+0x17d/0x250
[ 32.465407] ? __wake_up_common_lock+0xcb/0x110
[ 32.465410] ? __wake_up_common+0x230/0x230
[ 32.465413] ? netlink_bind+0x3e1/0x490
[ 32.465416] ? netlink_setsockopt+0x540/0x540
[ 32.465420] ? __fget_light+0x9c/0xf0
[ 32.465423] ? sockfd_lookup_light+0x8c/0xb0
[ 32.465426] __sys_sendmsg+0xa5/0x110
[ 32.465429] ? __ia32_sys_shutdown+0x30/0x30
[ 32.465432] ? __fd_install+0xe1/0x2c0
[ 32.465435] ? lockdep_hardirqs_off+0xb5/0x100
[ 32.465438] ? mark_held_locks+0x24/0x90
[ 32.465441] ? do_syscall_64+0xf/0x270
[ 32.465444] do_syscall_64+0x63/0x270
[ 32.465448] entry_SYSCALL_64_after_hwframe+0x49/0xbe
Fix the issue unscheduling neigh_timer if selected entry is in 'IN_TIMER'
receiving a netlink request with NTF_USE flag set
Reported-by: Marek Majkowski <marek@cloudflare.com>
Fixes: 0c5c2d308906 ("neigh: Allow for user space users of the neighbour table")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v2:
- remove check_timer flag and run neigh_del_timer directly
Changes since v1:
- fix compilation errors defining neigh_event_send_check_timer routine
---
net/core/neighbour.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 742cea4ce72e..0dfc97bc8760 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1124,6 +1124,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
atomic_set(&neigh->probes,
NEIGH_VAR(neigh->parms, UCAST_PROBES));
+ neigh_del_timer(neigh);
neigh->nud_state = NUD_INCOMPLETE;
neigh->updated = now;
next = now + max(NEIGH_VAR(neigh->parms, RETRANS_TIME),
@@ -1140,6 +1141,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
}
} else if (neigh->nud_state & NUD_STALE) {
neigh_dbg(2, "neigh %p is delayed\n", neigh);
+ neigh_del_timer(neigh);
neigh->nud_state = NUD_DELAY;
neigh->updated = jiffies;
neigh_add_timer(neigh, jiffies +
--
2.21.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: neigh: fix multiple neigh timer scheduling
2019-07-14 21:36 [PATCH net v3] net: neigh: fix multiple neigh timer scheduling Lorenzo Bianconi
@ 2019-07-15 16:32 ` David Ahern
2019-07-15 18:04 ` David Miller
2019-07-21 13:46 ` Julian Anastasov
2 siblings, 0 replies; 5+ messages in thread
From: David Ahern @ 2019-07-15 16:32 UTC (permalink / raw)
To: Lorenzo Bianconi, davem; +Cc: netdev, marek
On 7/14/19 3:36 PM, Lorenzo Bianconi wrote:
> Neigh timer can be scheduled multiple times from userspace adding
> multiple neigh entries and forcing the neigh timer scheduling passing
> NTF_USE in the netlink requests.
> This will result in a refcount leak and in the following dump stack:
>
...
>
> Fix the issue unscheduling neigh_timer if selected entry is in 'IN_TIMER'
> receiving a netlink request with NTF_USE flag set
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Fixes: 0c5c2d308906 ("neigh: Allow for user space users of the neighbour table")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - remove check_timer flag and run neigh_del_timer directly
> Changes since v1:
> - fix compilation errors defining neigh_event_send_check_timer routine
> ---
> net/core/neighbour.c | 2 ++
> 1 file changed, 2 insertions(+)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: neigh: fix multiple neigh timer scheduling
2019-07-14 21:36 [PATCH net v3] net: neigh: fix multiple neigh timer scheduling Lorenzo Bianconi
2019-07-15 16:32 ` David Ahern
@ 2019-07-15 18:04 ` David Miller
2019-07-21 13:46 ` Julian Anastasov
2 siblings, 0 replies; 5+ messages in thread
From: David Miller @ 2019-07-15 18:04 UTC (permalink / raw)
To: lorenzo.bianconi; +Cc: netdev, dsahern, marek
From: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
Date: Sun, 14 Jul 2019 23:36:11 +0200
> Neigh timer can be scheduled multiple times from userspace adding
> multiple neigh entries and forcing the neigh timer scheduling passing
> NTF_USE in the netlink requests.
> This will result in a refcount leak and in the following dump stack:
...
> Fix the issue unscheduling neigh_timer if selected entry is in 'IN_TIMER'
> receiving a netlink request with NTF_USE flag set
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Fixes: 0c5c2d308906 ("neigh: Allow for user space users of the neighbour table")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - remove check_timer flag and run neigh_del_timer directly
> Changes since v1:
> - fix compilation errors defining neigh_event_send_check_timer routine
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: neigh: fix multiple neigh timer scheduling
2019-07-14 21:36 [PATCH net v3] net: neigh: fix multiple neigh timer scheduling Lorenzo Bianconi
2019-07-15 16:32 ` David Ahern
2019-07-15 18:04 ` David Miller
@ 2019-07-21 13:46 ` Julian Anastasov
2019-07-30 15:19 ` Lorenzo Bianconi
2 siblings, 1 reply; 5+ messages in thread
From: Julian Anastasov @ 2019-07-21 13:46 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: davem, netdev, dsahern, marek
Hello,
On Sun, 14 Jul 2019, Lorenzo Bianconi wrote:
> Neigh timer can be scheduled multiple times from userspace adding
If the garbage comes from ndm_state, why we should create
a patch that just covers the problem?:
State: INCOMPLETE, STALE, FAILED, 0x8400 (0x8425)
User space is trying to create entry that is both
STALE (no timer) and INCOMPLETE (with timer). So, in the
2nd NL message __neigh_event_send() detects timer with NUD_STALE
bit. What if this 2nd message never comes? Such inconsistence
between nud_state and the timer can trigger other bugs in
other functions.
May be we just need to restrict ndm_state and to drop
this patch, for example, by adding checks in __neigh_update():
if (!(flags & NEIGH_UPDATE_F_ADMIN) &&
(old & (NUD_NOARP | NUD_PERMANENT)))
goto out;
+ /* State must be single bit or 0 */
+ if (new & (new - 1))
+ goto out;
if (neigh->dead) {
If needed, this check can be moved only for ndm_state
in neigh_add().
> multiple neigh entries and forcing the neigh timer scheduling passing
> NTF_USE in the netlink requests.
> This will result in a refcount leak and in the following dump stack:
It is a single create with multiple bits in state with following
__neigh_event_send(). And who knows, this bug may exist even in Linux 2.2
and below...
> [ 32.465295] NEIGH: BUG, double timer add, state is 8
> [ 32.465308] CPU: 0 PID: 416 Comm: double_timer_ad Not tainted 5.2.0+ #65
> [ 32.465311] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.12.0-2.fc30 04/01/2014
> [ 32.465313] Call Trace:
> [ 32.465318] dump_stack+0x7c/0xc0
> [ 32.465323] __neigh_event_send+0x20c/0x880
> [ 32.465326] ? ___neigh_create+0x846/0xfb0
> [ 32.465329] ? neigh_lookup+0x2a9/0x410
> [ 32.465332] ? neightbl_fill_info.constprop.0+0x800/0x800
> [ 32.465334] neigh_add+0x4f8/0x5e0
> [ 32.465337] ? neigh_xmit+0x620/0x620
> [ 32.465341] ? find_held_lock+0x85/0xa0
> [ 32.465345] rtnetlink_rcv_msg+0x204/0x570
> [ 32.465348] ? rtnl_dellink+0x450/0x450
> [ 32.465351] ? mark_held_locks+0x90/0x90
> [ 32.465354] ? match_held_lock+0x1b/0x230
> [ 32.465357] netlink_rcv_skb+0xc4/0x1d0
> [ 32.465360] ? rtnl_dellink+0x450/0x450
> [ 32.465363] ? netlink_ack+0x420/0x420
> [ 32.465366] ? netlink_deliver_tap+0x115/0x560
> [ 32.465369] ? __alloc_skb+0xc9/0x2f0
> [ 32.465372] netlink_unicast+0x270/0x330
> [ 32.465375] ? netlink_attachskb+0x2f0/0x2f0
> [ 32.465378] netlink_sendmsg+0x34f/0x5a0
> [ 32.465381] ? netlink_unicast+0x330/0x330
> [ 32.465385] ? move_addr_to_kernel.part.0+0x20/0x20
> [ 32.465388] ? netlink_unicast+0x330/0x330
> [ 32.465391] sock_sendmsg+0x91/0xa0
> [ 32.465394] ___sys_sendmsg+0x407/0x480
> [ 32.465397] ? copy_msghdr_from_user+0x200/0x200
> [ 32.465401] ? _raw_spin_unlock_irqrestore+0x37/0x40
> [ 32.465404] ? lockdep_hardirqs_on+0x17d/0x250
> [ 32.465407] ? __wake_up_common_lock+0xcb/0x110
> [ 32.465410] ? __wake_up_common+0x230/0x230
> [ 32.465413] ? netlink_bind+0x3e1/0x490
> [ 32.465416] ? netlink_setsockopt+0x540/0x540
> [ 32.465420] ? __fget_light+0x9c/0xf0
> [ 32.465423] ? sockfd_lookup_light+0x8c/0xb0
> [ 32.465426] __sys_sendmsg+0xa5/0x110
> [ 32.465429] ? __ia32_sys_shutdown+0x30/0x30
> [ 32.465432] ? __fd_install+0xe1/0x2c0
> [ 32.465435] ? lockdep_hardirqs_off+0xb5/0x100
> [ 32.465438] ? mark_held_locks+0x24/0x90
> [ 32.465441] ? do_syscall_64+0xf/0x270
> [ 32.465444] do_syscall_64+0x63/0x270
> [ 32.465448] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Fix the issue unscheduling neigh_timer if selected entry is in 'IN_TIMER'
> receiving a netlink request with NTF_USE flag set
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Fixes: 0c5c2d308906 ("neigh: Allow for user space users of the neighbour table")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> Changes since v2:
> - remove check_timer flag and run neigh_del_timer directly
> Changes since v1:
> - fix compilation errors defining neigh_event_send_check_timer routine
> ---
> net/core/neighbour.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 742cea4ce72e..0dfc97bc8760 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1124,6 +1124,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
>
> atomic_set(&neigh->probes,
> NEIGH_VAR(neigh->parms, UCAST_PROBES));
> + neigh_del_timer(neigh);
> neigh->nud_state = NUD_INCOMPLETE;
> neigh->updated = now;
> next = now + max(NEIGH_VAR(neigh->parms, RETRANS_TIME),
> @@ -1140,6 +1141,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
> }
> } else if (neigh->nud_state & NUD_STALE) {
> neigh_dbg(2, "neigh %p is delayed\n", neigh);
> + neigh_del_timer(neigh);
> neigh->nud_state = NUD_DELAY;
> neigh->updated = jiffies;
> neigh_add_timer(neigh, jiffies +
> --
> 2.21.0
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net v3] net: neigh: fix multiple neigh timer scheduling
2019-07-21 13:46 ` Julian Anastasov
@ 2019-07-30 15:19 ` Lorenzo Bianconi
0 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2019-07-30 15:19 UTC (permalink / raw)
To: Julian Anastasov; +Cc: davem, netdev, dsahern, marek
[-- Attachment #1: Type: text/plain, Size: 5867 bytes --]
>
> Hello,
Hi Julian,
sorry for the late reply, I was AFK
>
> On Sun, 14 Jul 2019, Lorenzo Bianconi wrote:
>
> > Neigh timer can be scheduled multiple times from userspace adding
>
> If the garbage comes from ndm_state, why we should create
> a patch that just covers the problem?:
>
> State: INCOMPLETE, STALE, FAILED, 0x8400 (0x8425)
>
> User space is trying to create entry that is both
> STALE (no timer) and INCOMPLETE (with timer). So, in the
> 2nd NL message __neigh_event_send() detects timer with NUD_STALE
> bit. What if this 2nd message never comes? Such inconsistence
> between nud_state and the timer can trigger other bugs in
> other functions.
I guess this patch is not harmful since it does nothing if we are not in
IN_TIMER and it fixes an issue if for some reason we hit this code and we
have already scheduled the neigh timer (other parts of the state machine
do the same). Anyway I agree we could add some sanity checks to inputs from
userspace.
Regards,
Lorenzo
>
> May be we just need to restrict ndm_state and to drop
> this patch, for example, by adding checks in __neigh_update():
>
> if (!(flags & NEIGH_UPDATE_F_ADMIN) &&
> (old & (NUD_NOARP | NUD_PERMANENT)))
> goto out;
> + /* State must be single bit or 0 */
> + if (new & (new - 1))
> + goto out;
> if (neigh->dead) {
>
> If needed, this check can be moved only for ndm_state
> in neigh_add().
>
> > multiple neigh entries and forcing the neigh timer scheduling passing
> > NTF_USE in the netlink requests.
> > This will result in a refcount leak and in the following dump stack:
>
> It is a single create with multiple bits in state with following
> __neigh_event_send(). And who knows, this bug may exist even in Linux 2.2
> and below...
>
> > [ 32.465295] NEIGH: BUG, double timer add, state is 8
> > [ 32.465308] CPU: 0 PID: 416 Comm: double_timer_ad Not tainted 5.2.0+ #65
> > [ 32.465311] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.12.0-2.fc30 04/01/2014
> > [ 32.465313] Call Trace:
> > [ 32.465318] dump_stack+0x7c/0xc0
> > [ 32.465323] __neigh_event_send+0x20c/0x880
> > [ 32.465326] ? ___neigh_create+0x846/0xfb0
> > [ 32.465329] ? neigh_lookup+0x2a9/0x410
> > [ 32.465332] ? neightbl_fill_info.constprop.0+0x800/0x800
> > [ 32.465334] neigh_add+0x4f8/0x5e0
> > [ 32.465337] ? neigh_xmit+0x620/0x620
> > [ 32.465341] ? find_held_lock+0x85/0xa0
> > [ 32.465345] rtnetlink_rcv_msg+0x204/0x570
> > [ 32.465348] ? rtnl_dellink+0x450/0x450
> > [ 32.465351] ? mark_held_locks+0x90/0x90
> > [ 32.465354] ? match_held_lock+0x1b/0x230
> > [ 32.465357] netlink_rcv_skb+0xc4/0x1d0
> > [ 32.465360] ? rtnl_dellink+0x450/0x450
> > [ 32.465363] ? netlink_ack+0x420/0x420
> > [ 32.465366] ? netlink_deliver_tap+0x115/0x560
> > [ 32.465369] ? __alloc_skb+0xc9/0x2f0
> > [ 32.465372] netlink_unicast+0x270/0x330
> > [ 32.465375] ? netlink_attachskb+0x2f0/0x2f0
> > [ 32.465378] netlink_sendmsg+0x34f/0x5a0
> > [ 32.465381] ? netlink_unicast+0x330/0x330
> > [ 32.465385] ? move_addr_to_kernel.part.0+0x20/0x20
> > [ 32.465388] ? netlink_unicast+0x330/0x330
> > [ 32.465391] sock_sendmsg+0x91/0xa0
> > [ 32.465394] ___sys_sendmsg+0x407/0x480
> > [ 32.465397] ? copy_msghdr_from_user+0x200/0x200
> > [ 32.465401] ? _raw_spin_unlock_irqrestore+0x37/0x40
> > [ 32.465404] ? lockdep_hardirqs_on+0x17d/0x250
> > [ 32.465407] ? __wake_up_common_lock+0xcb/0x110
> > [ 32.465410] ? __wake_up_common+0x230/0x230
> > [ 32.465413] ? netlink_bind+0x3e1/0x490
> > [ 32.465416] ? netlink_setsockopt+0x540/0x540
> > [ 32.465420] ? __fget_light+0x9c/0xf0
> > [ 32.465423] ? sockfd_lookup_light+0x8c/0xb0
> > [ 32.465426] __sys_sendmsg+0xa5/0x110
> > [ 32.465429] ? __ia32_sys_shutdown+0x30/0x30
> > [ 32.465432] ? __fd_install+0xe1/0x2c0
> > [ 32.465435] ? lockdep_hardirqs_off+0xb5/0x100
> > [ 32.465438] ? mark_held_locks+0x24/0x90
> > [ 32.465441] ? do_syscall_64+0xf/0x270
> > [ 32.465444] do_syscall_64+0x63/0x270
> > [ 32.465448] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > Fix the issue unscheduling neigh_timer if selected entry is in 'IN_TIMER'
> > receiving a netlink request with NTF_USE flag set
> >
> > Reported-by: Marek Majkowski <marek@cloudflare.com>
> > Fixes: 0c5c2d308906 ("neigh: Allow for user space users of the neighbour table")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> > ---
> > Changes since v2:
> > - remove check_timer flag and run neigh_del_timer directly
> > Changes since v1:
> > - fix compilation errors defining neigh_event_send_check_timer routine
> > ---
> > net/core/neighbour.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 742cea4ce72e..0dfc97bc8760 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -1124,6 +1124,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
> >
> > atomic_set(&neigh->probes,
> > NEIGH_VAR(neigh->parms, UCAST_PROBES));
> > + neigh_del_timer(neigh);
> > neigh->nud_state = NUD_INCOMPLETE;
> > neigh->updated = now;
> > next = now + max(NEIGH_VAR(neigh->parms, RETRANS_TIME),
> > @@ -1140,6 +1141,7 @@ int __neigh_event_send(struct neighbour *neigh, struct sk_buff *skb)
> > }
> > } else if (neigh->nud_state & NUD_STALE) {
> > neigh_dbg(2, "neigh %p is delayed\n", neigh);
> > + neigh_del_timer(neigh);
> > neigh->nud_state = NUD_DELAY;
> > neigh->updated = jiffies;
> > neigh_add_timer(neigh, jiffies +
> > --
> > 2.21.0
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-30 15:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-14 21:36 [PATCH net v3] net: neigh: fix multiple neigh timer scheduling Lorenzo Bianconi
2019-07-15 16:32 ` David Ahern
2019-07-15 18:04 ` David Miller
2019-07-21 13:46 ` Julian Anastasov
2019-07-30 15:19 ` Lorenzo Bianconi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.