| 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: <1315461572.2532.7.camel@edumazet-laptop>
Date: Thu, 08 Sep 2011 07:59:32 +0200
From: Eric Dumazet <eric.dumazet@...il.com>
To: Tim Chen <tim.c.chen@...ux.intel.com>
Cc: "Yan, Zheng" <zheng.z.yan@...el.com>,
"sedat.dilek@...il.com" <sedat.dilek@...il.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
Le mercredi 07 septembre 2011 à 14:06 -0700, Tim Chen a écrit :
> On Thu, 2011-09-08 at 08:27 +0800, Yan, Zheng wrote:
>
> > > err = -EPIPE;
> > > out_err:
> > > - if (skb == NULL)
> > > + if (!steal_refs)
> > > scm_destroy(siocb->scm);
> >
> > I think we should call scm_release() here in the case of
> > steal_refs == true. Otherwise siocb->scm->fp may leak.
>
> Yan Zheng,
>
> I've updated the patch. If it looks good to you now, can you add your
> Signed-off-by to the patch. Pending Sedat's testing on this patch,
> I think it is good to go.
>
> 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..47780dc 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,11 +1644,14 @@ 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;
> + goto out_err;
> }
> max_level = err + 1;
> fds_sent = true;
> @@ -1654,7 +1659,7 @@ static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
> err = memcpy_fromiovec(skb_put(skb, size), msg->msg_iov, size);
> if (err) {
> kfree_skb(skb);
> - goto out;
> + goto out_err;
> }
>
> unix_state_lock(other);
> @@ -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);
> @@ -1687,9 +1692,10 @@ pipe_err:
> send_sig(SIGPIPE, current, 0);
> err = -EPIPE;
> out_err:
> - if (skb == NULL)
> + if (steal_refs)
> + scm_release(siocb->scm);
> + else
> scm_destroy(siocb->scm);
> -out:
> siocb->scm = NULL;
> return sent ? : err;
> }
>
>
>
I dont think this patch is good.
Sedat traces have nothing to do with af_unix.
Once unix_scm_to_skb() was called and successful, and steal_refs is true
our refs are attached to this skb. They will be released by
skb_free(skb). Same for fp : They either were sent in a previous skb or
this one.
This is why the "goto out;" was OK.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists