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 for Android: free password hash cracker in your pocket

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

Message-ID: <883c86d2-49b0-4d5e-a360-286865890180@stanley.mountain>
Date: Fri, 16 Aug 2024 20:10:21 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Rao Shoaib <rao.shoaib@...cle.com>
Cc: netdev@...r.kernel.org
Subject: Re: [bug report] af_unix: Add OOB support

On Fri, Aug 16, 2024 at 09:50:56AM -0700, Rao Shoaib wrote:
> 
> 
> 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.
> 

No, I was suggesting maybe we *should* return NULL.  The question is why are we
returning a freed skb pointer?

regards,
dan carpenter


> 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