Message-ID: <CA+icZUVqJVn4ONuSDV5YdgpEcmetVMM9X=Sxt2EqM8qdegMnjQ@mail.gmail.com>
Date:	Wed, 7 Sep 2011 22:12:09 +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 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 -

View attachment "dmesg.txt" of type "text/plain" (85521 bytes)

Powered by blists - more mailing lists