[PATCH net] net: Check for fullsock in sock_i_uid()

[PATCH net] net: Check for fullsock in sock_i_uid()

[Subash Abhinov Kasiviswanathan]

sock_i_uid() acquires the sk_callback_lock which does not exist
for sockets in TCP_NEW_SYN_RECV state. This results in errors
showing up as spinlock bad magic.

Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Cc: Eric Dumazet <edumazet@google.com>
---
 net/core/sock.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index c73e28f..af15ef0 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1727,7 +1727,10 @@ void sock_efree(struct sk_buff *skb)
 
 kuid_t sock_i_uid(struct sock *sk)
 {
-	kuid_t uid;
+	kuid_t uid = GLOBAL_ROOT_UID;
+
+	if (!sk_fullsock(sk))
+		return uid;
 
 	read_lock_bh(&sk->sk_callback_lock);
 	uid = sk->sk_socket ? SOCK_INODE(sk->sk_socket)->i_uid : GLOBAL_ROOT_UID;
-- 
1.9.1

Re: [PATCH net] net: Check for fullsock in sock_i_uid()

[Eric Dumazet]

On Tue, 2016-11-01 at 23:27 -0600, Subash Abhinov Kasiviswanathan wrote:
> sock_i_uid() acquires the sk_callback_lock which does not exist
> for sockets in TCP_NEW_SYN_RECV state. This results in errors
> showing up as spinlock bad magic.
> 
> Signed-off-by: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
> Cc: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/sock.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index c73e28f..af15ef0 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1727,7 +1727,10 @@ void sock_efree(struct sk_buff *skb)
>  
>  kuid_t sock_i_uid(struct sock *sk)
>  {
> -	kuid_t uid;
> +	kuid_t uid = GLOBAL_ROOT_UID;
> +
> +	if (!sk_fullsock(sk))
> +		return uid;
>  
>  	read_lock_bh(&sk->sk_callback_lock);
>  	uid = sk->sk_socket ? SOCK_INODE(sk->sk_socket)->i_uid : GLOBAL_ROOT_UID;


This would be a bug in the caller.

Can you give us the complete stack trace leading to the problem you
had ?

Thanks !

Re: [PATCH net] net: Check for fullsock in sock_i_uid()

[subashab]

> This would be a bug in the caller.
> 
> Can you give us the complete stack trace leading to the problem you
> had ?
> 
> Thanks !

Thanks Eric for the clarification. In that case, the bug is in the 
IDLETIMER target in Android kernel.
https://android.googlesource.com/kernel/common/+/android-4.4/net/netfilter/xt_IDLETIMER.c#356

Here is the call stack.

-003|rwlock_bug(?, ?)
-004|arch_read_lock(inline)
-004|do_raw_read_lock(lock = 0xFFFFFFC0354E79C8)
-005|raw_read_lock_bh(lock = 0xFFFFFFC0354E79C8)
-006|sock_i_uid(sk = 0xFFFFFFC0354E77B0)
-007|from_kuid_munged(inline)
-007|reset_timer(info = 0xFFFFFFC04D17D218, skb = 0xFFFFFFC018AB98C0)
-008|idletimer_tg_target(skb = 0xFFFFFFC018AB98C0, ?)
-009|ipt_do_table(skb = 0xFFFFFFC018AB98C0, state = 0xFFFFFFC0017E6F30, 
?)
-010|iptable_mangle_hook(?, skb = 0xFFFFFFC018AB98C0, state = 
0xFFFFFFC0017E6F30)
-011|nf_iterate(head = 0xFFFFFFC0019D55B8, skb = 0xFFFFFFC018AB98C0, 
state = 0xFFFFFFC0017E6F30, elemp =
-012|nf_hook_slow(skb = 0xFFFFFFC018AB98C0, state = 0xFFFFFFC0017E6F30)
-013|NF_HOOK_COND(inline)
-013|ip_output(net = 0xFFFFFFC0019D4B00, sk = 0xFFFFFFC0354E77B0, skb = 
0xFFFFFFC018AB98C0)
-014|ip_local_out(net = 0xFFFFFFC0019D4B00, sk = 0xFFFFFFC0354E77B0, skb 
= 0xFFFFFFC018AB98C0)
-015|ip_build_and_send_pkt(skb = 0xFFFFFFC018AB98C0, sk = 
0xFFFFFFC023F2E880, saddr = 1688053952, daddr =
-016|tcp_v4_send_synack(sk = 0xFFFFFFC023F2E880, ?, ?, req = 
0xFFFFFFC0354E77B0, foc = 0xFFFFFFC0017E7110
-017|atomic_sub_return(inline)
-017|reqsk_put(inline)
-017|tcp_conn_request(?, af_ops = 0xFFFFFFC001080FC8, sk = 
0xFFFFFFC023F2E880, ?)
-018|tcp_v4_conn_request(?, ?)
-019|tcp_rcv_state_process(sk = 0xFFFFFFC023F2E880, skb = 
0xFFFFFFC018ABAD00)
-020|tcp_v4_do_rcv(sk = 0xFFFFFFC023F2E880, skb = 0xFFFFFFC018ABAD00)
-021|tcp_v4_rcv(skb = 0xFFFFFFC018ABAD00)
-022|ip_local_deliver_finish(net = 0xFFFFFFC0019D4B00, ?, skb = 
0xFFFFFFC018ABAD00)
-023|NF_HOOK_THRESH(inline)
-023|NF_HOOK(inline)
-023|ip_local_deliver(skb = 0xFFFFFFC018ABAD00)
-024|ip_rcv_finish(net = 0xFFFFFFC0019D4B00, ?, skb = 
0xFFFFFFC018ABAD00)
-025|NF_HOOK_THRESH(inline)
-025|NF_HOOK(inline)
-025|ip_rcv(skb = 0xFFFFFFC018ABAD00, dev = 0xFFFFFFC023474000, ?, ?)
-026|deliver_skb(inline)
-026|deliver_ptype_list_skb(inline)
-026|__netif_receive_skb_core(skb = 0x0A73, pfmemalloc = FALSE)
-027|__netif_receive_skb(skb = 0xFFFFFFC0BA455D40)
-028|netif_receive_skb_internal(skb = 0xFFFFFFC0BA455D40)
-029|netif_receive_skb(skb = 0xFFFFFFC0BA455D40)

Re: [PATCH net] net: Check for fullsock in sock_i_uid()

[Eric Dumazet]

On Wed, 2016-11-02 at 11:05 -0600, subashab@codeaurora.org wrote:
> > This would be a bug in the caller.
> > 
> > Can you give us the complete stack trace leading to the problem you
> > had ?
> > 
> > Thanks !
> 
> Thanks Eric for the clarification. In that case, the bug is in the 
> IDLETIMER target in Android kernel.
> https://android.googlesource.com/kernel/common/+/android-4.4/net/netfilter/xt_IDLETIMER.c#356
> 
> Here is the call stack.

Sure, please fix Android, and not add ugly work around in linux kernel.

Lorenzo, have'nt you already fixed all these bugs ?

if (skb && skb->sk)
	timer->uid = from_kuid_munged(current_user_ns(),
				 sock_i_uid(skb_to_full_sk(skb)));

Thanks

Re: [PATCH net] net: Check for fullsock in sock_i_uid()

[Lorenzo Colitti]

On Thu, Nov 3, 2016 at 2:18 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Lorenzo, have'nt you already fixed all these bugs ?

Not yet. There's still a fair bit of out-of-tree code left. Other than
per-UID routing, xt_qtaguid is the big one, but there's also xt_quota2
and xt_idletimer. to fix.

> if (skb && skb->sk)
>         timer->uid = from_kuid_munged(current_user_ns(),
>                                  sock_i_uid(skb_to_full_sk(skb)));

Thanks! I didn't know about skb_to_full_sk.