| Message ID | 1315429583.2361.3.camel@schen9-mobl |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2011-09-07 at 14:06 -0700, Tim Chen wrote: > 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 Oops, I've forgotten to add Eric's previous Signed-off-by in my latest patch log. David, please add that when you pick up the patch. Once Yan Zheng added his sign off and Sedat tested the patch, I think it will be good to go. Thanks. Tim -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/08/2011 05:06 AM, Tim Chen wrote: > 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@linux.intel.com> > Reported-by: Jiri Slaby <jirislaby@gmail.com> > Tested-by: Sedat Dilek <sedat.dilek@googlemail.com> > Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.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; > } > > > Signed-off-by: Zheng Yan <zheng.z.yan@intel.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@linux.intel.com> > Reported-by: Jiri Slaby <jirislaby@gmail.com> > Tested-by: Sedat Dilek <sedat.dilek@googlemail.com> > Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mercredi 07 septembre 2011 à 14:15 -0700, Tim Chen a écrit : > Oops, I've forgotten to add Eric's previous Signed-off-by in my latest > patch log. David, please add that when you pick up the patch. > Once Yan Zheng added his sign off and Sedat tested the patch, I think it > will be good to go. Tim, as soon as you post another patch version, you must remove all prior Signed-off-by, and only add yours. So it was fine to do so. By the way your last version introduce a new bug, so I would rather NACK it ;) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/08/2011 01:59 PM, Eric Dumazet wrote: > 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@linux.intel.com> >> Reported-by: Jiri Slaby <jirislaby@gmail.com> >> Tested-by: Sedat Dilek <sedat.dilek@googlemail.com> >> Tested-by: Valdis Kletnieks <Valdis.Kletnieks@vt.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. > I don't think so. unix_scm_to_skb() calls unix_attach_fds(), it always duplicates scm->fp. Regards -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }