| 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
| ||
|
Message-ID: <07256914-a5c7-4aee-9880-6066c7dcceb0@oracle.com>
Date: Fri, 16 Aug 2024 10:28:14 -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 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.
Regards,
Shoaib
>
>
>> 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
Powered by blists - more mailing lists