| 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
| ||
|
Message-ID: <20190730151944.GB18771@localhost.localdomain>
Date: Tue, 30 Jul 2019 17:19:44 +0200
From: Lorenzo Bianconi <lorenzo.bianconi@...hat.com>
To: Julian Anastasov <ja@....bg>
Cc: davem@...emloft.net, netdev@...r.kernel.org, dsahern@...il.com,
marek@...udflare.com
Subject: Re: [PATCH net v3] net: neigh: fix multiple neigh timer scheduling
>
> 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@...udflare.com>
> > Fixes: 0c5c2d308906 ("neigh: Allow for user space users of the neighbour table")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@...hat.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@....bg>
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists