| 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: <CA+icZUW5Biu8cMhaJcz=MwtMOVn-NFD92PTpT0-DfEDKrRmaUQ@mail.gmail.com>
Date: Wed, 7 Sep 2011 22:30:42 +0200
From: Sedat Dilek <sedat.dilek@...glemail.com>
To: Tim Chen <tim.c.chen@...ux.intel.com>
Cc: Eric Dumazet <eric.dumazet@...il.com>,
"Yan, Zheng" <zheng.z.yan@...el.com>,
"Yan, Zheng" <yanzheng@...n.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"sfr@...b.auug.org.au" <sfr@...b.auug.org.au>,
"jirislaby@...il.com" <jirislaby@...il.com>,
"Shi, Alex" <alex.shi@...el.com>,
Valdis Kletnieks <Valdis.Kletnieks@...edu>
Subject: Re: [PATCH -next v2] unix stream: Fix use-after-free crashes
On Wed, Sep 7, 2011 at 10:12 PM, Sedat Dilek <sedat.dilek@...glemail.com> wrote:
> On Wed, Sep 7, 2011 at 2:01 PM, Tim Chen <tim.c.chen@...ux.intel.com> wrote:
>> On Wed, 2011-09-07 at 09:45 +0200, Eric Dumazet wrote:
>>> Le mercredi 07 septembre 2011 à 13:20 +0800, Yan, Zheng a écrit :
>>>
>>> > Is code like this OK? Thanks
>>> > ---
>>> > if (sent + size < len) {
>>> > /* Only send the fds in the first buffer */
>>> > /* get additional ref if more skbs will be created */
>>> > err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, true);
>>> > } else {
>>> > err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, false);
>>> > ref_avail = false;
>>> > }
>>> >
>>> >
>>>
>>> Whats wrong with using ref_avail in the unix_scm_to_skb() call itself ?
>>>
>>> something like :
>>>
>>
>> Eric,
>>
>> Your updated patch looks good when I tested it on my side. It makes the
>> patch much more readable. If this patch looks good with you and Yan
>> Zheng, can you and Yan Zheng add your Signed-off-by to the patch?
>>
>> Jiri, Sedat or Valdis, if you can verify that the patch fixed commit
>> 0856a30409, that will be appreciated.
>>
>> Eric, are you planning to do a fast path patch that doesn't do pid ref
>> for the case where CONFIG_PID_NS is not set?
>>
>> Thanks.
>>
>> Tim
>>
>> ---
>>
>> Commit 0856a30409 (Scm: Remove unnecessary pid & credential references
>> in Unix socket's send and receive path) introduced a use-after-free bug.
>> The sent skbs from unix_stream_sendmsg could be consumed and destructed
>> by the receive side, removing all references to the credentials,
>> before the send side has finished sending out all
>> packets. However, send side could continue to consturct new packets in the
>> stream, using credentials that have lost its last reference and been
>> freed.
>>
>> In this fix, we don't steal the reference to credentials we have obtained
>> in scm_send at beginning of unix_stream_sendmsg, till we've reached
>> the last packet. This fixes the problem in commit 0856a30409.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
>> Reported-by: Jiri Slaby <jirislaby@...il.com>
>> Tested-by: Sedat Dilek <sedat.dilek@...glemail.com>
>> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@...edu>
>> ---
>>
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index 136298c..4a324a0 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -1383,10 +1383,11 @@ static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
>> }
>>
>> static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb,
>> - bool send_fds, bool ref)
>> + bool send_fds, bool steal_refs)
>> {
>> int err = 0;
>> - if (ref) {
>> +
>> + if (!steal_refs) {
>> UNIXCB(skb).pid = get_pid(scm->pid);
>> UNIXCB(skb).cred = get_cred(scm->cred);
>> } else {
>> @@ -1458,7 +1459,7 @@ static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> if (skb == NULL)
>> goto out;
>>
>> - err = unix_scm_to_skb(siocb->scm, skb, true, false);
>> + err = unix_scm_to_skb(siocb->scm, skb, true, true);
>> if (err < 0)
>> goto out_free;
>> max_level = err + 1;
>> @@ -1581,6 +1582,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> int sent = 0;
>> struct scm_cookie tmp_scm;
>> bool fds_sent = false;
>> + bool steal_refs = false;
>> int max_level;
>>
>> if (NULL == siocb->scm)
>> @@ -1642,8 +1644,11 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> size = min_t(int, size, skb_tailroom(skb));
>>
>>
>> - /* Only send the fds and no ref to pid in the first buffer */
>> - err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, fds_sent);
>> + /* Only send the fds in first buffer
>> + * Last buffer can steal our references to pid/cred
>> + */
>> + steal_refs = (sent + size >= len);
>> + err = unix_scm_to_skb(siocb->scm, skb, !fds_sent, steal_refs);
>> if (err < 0) {
>> kfree_skb(skb);
>> goto out;
>> @@ -1671,7 +1676,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
>> sent += size;
>> }
>>
>> - if (skb)
>> + if (steal_refs)
>> scm_release(siocb->scm);
>> else
>> scm_destroy(siocb->scm);
>>
>>
>>
>
> Replaced v2 with this patch (against next-20110831), I see now some
> different call-traces which I did not see with v1 or v2.
> Can't say if it's related to the new patch or not.
> ( dmesg attached. )
>
> - Sedat -
>
Call-traces seem to go away when adding "irqpoll" to Kernel command line.
( See dmesg_irqpoll.txt )
- Sedat -
View attachment "dmesg_irqpoll.txt" of type "text/plain" (55516 bytes)
Powered by blists - more mailing lists