netdev - Re: [bug report] af_unix: Add OOB support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c3c2b2e-4fd4-498c-8347-1a82b0b770a6@oracle.com>
Date: Fri, 16 Aug 2024 09:50:56 -0700
From: Rao Shoaib <rao.shoaib@...cle.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: netdev@...r.kernel.org
Subject: Re: [bug report] af_unix: Add OOB support



On 8/16/24 07:22, Dan Carpenter wrote:
> Hello Rao Shoaib,
> 
> Commit 314001f0bf92 ("af_unix: Add OOB support") from Aug 1, 2021
> (linux-next), leads to the following Smatch static checker warning:
> 
> 	net/unix/af_unix.c:2718 manage_oob()
> 	warn: 'skb' was already freed. (line 2699)
> 
> net/unix/af_unix.c
>     2665 static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
>     2666                                   int flags, int copied)
>     2667 {
>     2668         struct unix_sock *u = unix_sk(sk);
>     2669 
>     2670         if (!unix_skb_len(skb)) {
>     2671                 struct sk_buff *unlinked_skb = NULL;
>     2672 
>     2673                 spin_lock(&sk->sk_receive_queue.lock);
>     2674 
>     2675                 if (copied && (!u->oob_skb || skb == u->oob_skb)) {
>     2676                         skb = NULL;
>     2677                 } else if (flags & MSG_PEEK) {
>     2678                         skb = skb_peek_next(skb, &sk->sk_receive_queue);
>     2679                 } else {
>     2680                         unlinked_skb = skb;
>     2681                         skb = skb_peek_next(skb, &sk->sk_receive_queue);
>     2682                         __skb_unlink(unlinked_skb, &sk->sk_receive_queue);
>     2683                 }
>     2684 
>     2685                 spin_unlock(&sk->sk_receive_queue.lock);
>     2686 
>     2687                 consume_skb(unlinked_skb);
>     2688         } else {
>     2689                 struct sk_buff *unlinked_skb = NULL;
>     2690 
>     2691                 spin_lock(&sk->sk_receive_queue.lock);
>     2692 
>     2693                 if (skb == u->oob_skb) {
>     2694                         if (copied) {
>     2695                                 skb = NULL;
>     2696                         } else if (!(flags & MSG_PEEK)) {
>     2697                                 if (sock_flag(sk, SOCK_URGINLINE)) {
>     2698                                         WRITE_ONCE(u->oob_skb, NULL);
>     2699                                         consume_skb(skb);
> 
> Why are we returning this freed skb?  It feels like we should return NULL.

Hi Dan,

manage_oob is called from the following code segment

#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
                if (skb) {
                        skb = manage_oob(skb, sk, flags, copied);
                        if (!skb && copied) {
                                unix_state_unlock(sk);
                                break;
                        }
                }
#endif

So skb can not be NULL when manage_oob is called. The code that you
pointed out may free the skb (if the refcnts were incorrect) but skb
would not be NULL. It seems to me that the checker is incorrect or maybe
there is a way that skb maybe NULL and I am just not seeing it.

If you can explain to me how skb can be NULL, I will be happy to fix the
issue.

Thanks for reporting.

Shoaib
 		
> 
>     2700                                 } else {
>     2701                                         __skb_unlink(skb, &sk->sk_receive_queue);
>     2702                                         WRITE_ONCE(u->oob_skb, NULL);
>     2703                                         unlinked_skb = skb;
>     2704                                         skb = skb_peek(&sk->sk_receive_queue);
>     2705                                 }
>     2706                         } else if (!sock_flag(sk, SOCK_URGINLINE)) {
>     2707                                 skb = skb_peek_next(skb, &sk->sk_receive_queue);
>     2708                         }
>     2709                 }
>     2710 
>     2711                 spin_unlock(&sk->sk_receive_queue.lock);
>     2712 
>     2713                 if (unlinked_skb) {
>     2714                         WARN_ON_ONCE(skb_unref(unlinked_skb));
>     2715                         kfree_skb(unlinked_skb);
>     2716                 }
>     2717         }
> --> 2718         return skb;
>                         ^^^
> 
>     2719 }
> 
> regards,
> dan carpenter