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

Message-ID: <44c91443-3ac0-4e67-8a56-57ae9e21d7db@stanley.mountain>
Date: Fri, 16 Aug 2024 17:22:29 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Rao Shoaib <rao.shoaib@...cle.com>
Cc: netdev@...r.kernel.org
Subject: [bug report] af_unix: Add OOB support

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.

    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