From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [bpf-next v2 1/2] bpf: skmsg, fix psock create on existing kcm/tls port Date: Thu, 18 Oct 2018 10:34:55 -0700 Message-ID: References: <1539840004-26433-1-git-send-email-john.fastabend@gmail.com> <1539840004-26433-2-git-send-email-john.fastabend@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org To: John Fastabend , ast@kernel.org, daniel@iogearbox.net Return-path: Received: from mail-pg1-f195.google.com ([209.85.215.195]:44659 "EHLO mail-pg1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726269AbeJSBg6 (ORCPT ); Thu, 18 Oct 2018 21:36:58 -0400 Received: by mail-pg1-f195.google.com with SMTP id g2-v6so14499227pgu.11 for ; Thu, 18 Oct 2018 10:34:58 -0700 (PDT) In-Reply-To: <1539840004-26433-2-git-send-email-john.fastabend@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 10/17/2018 10:20 PM, John Fastabend wrote: > Before using the psock returned by sk_psock_get() when adding it to a > sockmap we need to ensure it is actually a sockmap based psock. > Previously we were only checking this after incrementing the reference > counter which was an error. This resulted in a slab-out-of-bounds > error when the psock was not actually a sockmap type. > > This moves the check up so the reference counter is only used > if it is a sockmap psock. > > Eric reported the following KASAN BUG, > > BUG: KASAN: slab-out-of-bounds in atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] > BUG: KASAN: slab-out-of-bounds in refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 > Read of size 4 at addr ffff88019548be58 by task syz-executor4/22387 > > CPU: 1 PID: 22387 Comm: syz-executor4 Not tainted 4.19.0-rc7+ #264 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113 > print_address_description.cold.8+0x9/0x1ff mm/kasan/report.c:256 > kasan_report_error mm/kasan/report.c:354 [inline] > kasan_report.cold.9+0x242/0x309 mm/kasan/report.c:412 > check_memory_region_inline mm/kasan/kasan.c:260 [inline] > check_memory_region+0x13e/0x1b0 mm/kasan/kasan.c:267 > kasan_check_read+0x11/0x20 mm/kasan/kasan.c:272 > atomic_read include/asm-generic/atomic-instrumented.h:21 [inline] > refcount_inc_not_zero_checked+0x97/0x2f0 lib/refcount.c:120 > sk_psock_get include/linux/skmsg.h:379 [inline] > sock_map_link.isra.6+0x41f/0xe30 net/core/sock_map.c:178 > sock_hash_update_common+0x19b/0x11e0 net/core/sock_map.c:669 > sock_hash_update_elem+0x306/0x470 net/core/sock_map.c:738 > map_update_elem+0x819/0xdf0 kernel/bpf/syscall.c:818 > > Signed-off-by: John Fastabend > Reported-by: Eric Dumazet > Fixes: 604326b41a6f ("bpf, sockmap: convert to generic sk_msg interface") > --- > include/linux/skmsg.h | 25 ++++++++++++++++++++----- > net/core/sock_map.c | 11 ++++++----- > 2 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/include/linux/skmsg.h b/include/linux/skmsg.h > index 677b673..f44ca6b 100644 > --- a/include/linux/skmsg.h > +++ b/include/linux/skmsg.h > @@ -275,11 +275,6 @@ static inline struct sk_psock *sk_psock(const struct sock *sk) > return rcu_dereference_sk_user_data(sk); > } > > -static inline bool sk_has_psock(struct sock *sk) > -{ > - return sk_psock(sk) != NULL && sk->sk_prot->recvmsg == tcp_bpf_recvmsg; > -} > - > static inline void sk_psock_queue_msg(struct sk_psock *psock, > struct sk_msg *msg) > { > @@ -379,6 +374,26 @@ static inline bool sk_psock_test_state(const struct sk_psock *psock, > return test_bit(bit, &psock->state); > } > > +static inline struct sk_psock *sk_psock_get_checked(struct sock *sk) > +{ > + struct sk_psock *psock; > + > + rcu_read_lock(); > + psock = sk_psock(sk); > + if (psock) { > + if (sk->sk_prot->recvmsg != tcp_bpf_recvmsg) { > + psock = ERR_PTR(-EBUSY); > + goto out; > + } > + > + if (!refcount_inc_not_zero(&psock->refcnt)) > + psock = NULL; Caller is using IS_ERR(), so you probably want to : psock = ERR_PTR(-E); > + } > +out: > + rcu_read_unlock(); > + return psock; > +} > + > static inline struct sk_psock *sk_psock_get(struct sock *sk) > { > struct sk_psock *psock; > diff --git a/net/core/sock_map.c b/net/core/sock_map.c > index 3c0e44c..be6092a 100644 > --- a/net/core/sock_map.c > +++ b/net/core/sock_map.c > @@ -175,12 +175,13 @@ static int sock_map_link(struct bpf_map *map, struct sk_psock_progs *progs, > } > } > > - psock = sk_psock_get(sk); > + psock = sk_psock_get_checked(sk); > + if (IS_ERR(psock)) { > + ret = PTR_ERR(psock); > + goto out_progs; > + } > + > if (psock) { > - if (!sk_has_psock(sk)) { > - ret = -EBUSY; > - goto out_progs; > - } > if ((msg_parser && READ_ONCE(psock->progs.msg_parser)) || > (skb_progs && READ_ONCE(psock->progs.skb_parser))) { > sk_psock_put(sk, psock); >