netdev - Re: [PATCH v2 net] af_unix: Clear stale u->oob

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: <472044aa-4427-40f0-9b9a-bce75d5c8aac@oracle.com>
Date: Fri, 5 Apr 2024 16:28:52 -0700
From: Rao Shoaib <rao.shoaib@...cle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        kuni1840@...il.com, netdev@...r.kernel.org, pabeni@...hat.com,
        syzbot+7f7f201cc2668a8fd169@...kaller.appspotmail.com
Subject: Re: [PATCH v2 net] af_unix: Clear stale u->oob_skb.



On 4/5/24 16:01, Kuniyuki Iwashima wrote:
> From: Rao Shoaib <rao.shoaib@...cle.com>
> Date: Fri, 5 Apr 2024 15:43:26 -0700
> [...]
>>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>>> index 5b41e2321209..d032eb5fa6df 100644
>>> --- a/net/unix/af_unix.c
>>> +++ b/net/unix/af_unix.c
>>> @@ -2665,7 +2665,9 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk,
>>>   				}
>>>   			} else if (!(flags & MSG_PEEK)) {
>>>   				skb_unlink(skb, &sk->sk_receive_queue);
>>> -				consume_skb(skb);
>>> +				WRITE_ONCE(u->oob_skb, NULL);
>>> +				if (!WARN_ON_ONCE(skb_unref(skb)))
>>> +					kfree_skb(skb);
>>>   				skb = skb_peek(&sk->sk_receive_queue);
>>>   			}
>>>   		}
>>
>> Isn't  consume_skb doing the same thing() ? .
> 
> Only if you disable CONFIG_TRACEPOINTS, otherwise each function
> uses different tracepoints, trace_consume_skb() vs trace_kfree_skb().
> 
> Here, we clearly drop the skb that's not received by user, so
> kfree_skb() should be used.

I won't get into the semantics of freed or consumed. So fine go with it. 
However if I was tracing I would just trace kfree_skb.
> 
> 
>> The only action needed is to clear out u->oob_skb -- No?
> 
> Also, queue_oob() now calls skb_get() and holds another refcnt,
> so skb_unref() is needed.

A quick look, does not show me that the proposed fix does anything 
different wrt to the refcnt. consume_skb() also calls skb_unref() and 
kfree_skb().

I am fine with this.

Reviewed-by: Rao shoaib <rao.shoaib@...cle.com>

Shoaib


> 
> BTW, do you know if MSG_OOB is actually used in production ?
> I don't know any real user other than syzbot.
> 
> P.S.
> Please send mail in plain text format as mailing list drops HTML.