netdev - Re: [PATCH v1] Remove zero length skb's when enqueuing new 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: <c9cdf55a-3d06-4e91-85bb-ec7e4f4ea92c@oracle.com>
Date: Tue, 10 Sep 2024 09:49:06 -0700
From: Shoaib Rao <rao.shoaib@...cle.com>
To: Simon Horman <horms@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, kuniyu@...zon.com, netdev@...r.kernel.org
Subject: Re: [PATCH v1] Remove zero length skb's when enqueuing new OOB



On 9/10/2024 6:44 AM, Simon Horman wrote:
> On Mon, Sep 09, 2024 at 05:28:54PM -0700, Rao Shoaib wrote:
>> 13:03 Recent tests show that AF_UNIX socket code does not handle
>> the following sequence properly
>>
>> Send OOB
>> Read OOB
>> Send OOB
>> Read (Without OOB flag)
>>
>> The last read returns the OOB byte, which is incorrect.
>> A following read with OOB flag returns EFAULT, which is also incorrect.
>>
>> In AF_UNIX, OOB byte is stored in a single skb, a pointer to the
>> skb is stored in the linux socket (oob_skb) and the skb is linked
>> in the socket's receive queue. Obviously, there are two refcnts on
>> the skb.
>>
>> If the byte is read as an OOB, there will be no remaining data and
>> regular read frees the skb in managge_oob() and moves to the next skb.
>> The bug was that the next skb could be an OOB byte, but the code did
>> not check that which resulted in a regular read, receiving the OOB byte.
>>
>> This patch adds code check the next skb obtained when a zero
>> length skb is freed.
>>
>> The patch also adds code to check and remove an skb in front
>> of about to be added OOB if it is a zero length skb.
>>
>> The cause of the last EFAULT was that the OOB byte had already been read
>> by the regular read but oob_skb was not cleared. This resulted in
>> __skb_datagram_iter() receiving a zero length skb to copy a byte from.
>> So EFAULT was returned.
>>
>> Fixes: 314001f0bf92 ("af_unix: Add OOB support")
>> Signed-off-by: Rao Shoaib <Rao.Shoaib@...cle.com>
> 
> Hi Rao,
> 
> This is not a proper review, I will leave that to Iwashima-san and others.
> 
> But I would like to note that as a fix for net it needs to be annotated as
> such.
> 
> 	Subject: [PATCH net v1] ...
> 
> Unfortunately while the patch applies to net it does not apply to net-next.
> But without the above annotation the CI did not know to apply the patch to
> net. So the CI can't process this patch.
> 
> I suggest posting a v2, targeted at net, after waiting for a review from
> Iwashima-san and others.
> 

Thanks for pointing out. It may not be necessary for me to post a v2 as 
another fix has been accepted but let's see.

Shoaib