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] [day] [month] [year] [list]

Message-ID: <20240816185149.31006-1-kuniyu@amazon.com>
Date: Fri, 16 Aug 2024 11:51:49 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <dan.carpenter@...aro.org>
CC: <netdev@...r.kernel.org>, <rao.shoaib@...cle.com>, <kuniyu@...zon.com>
Subject: Re: [bug report] af_unix: Add OOB support

From: Dan Carpenter <dan.carpenter@...aro.org>
Date: Fri, 16 Aug 2024 21:00:38 +0300
> 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!

This reminds me of my local patch that removes the additinal refcnt for
the OOB skb.

I'll post it officially.

Thanks!