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: <f2f90f1f-f231-42ad-b94e-6960ef248a20@stanley.mountain>
Date: Fri, 16 Aug 2024 21:00:38 +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 10:28:14AM -0700, Rao Shoaib wrote:
> 
> 
> On 8/16/24 10:10, Dan Carpenter wrote:
> > 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
> 
> We are not returning a freed skb pointer. The refcnt's protect the skb
> from being freed. Now if somehow the refcnts are wrong and the skb gets
> freed, that is a different issue and is a bug.
> 

Ah ok.  Thanks!

regards,
dan carpenter