Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys/net Fix not to use PPPOE_UNLOCK before acccess to pppoe_...



details:   https://anonhg.NetBSD.org/src/rev/907cb28f04b7
branches:  trunk
changeset: 362626:907cb28f04b7
user:      yamaguchi <yamaguchi%NetBSD.org@localhost>
date:      Mon Jun 18 09:49:05 2018 +0000

description:
Fix not to use PPPOE_UNLOCK before acccess to pppoe_softc
to avoid a race condition

According to the locking order of pppoe(4), the access to
pppoe_softc has to follow 5 steps as below.

1. aquire pppoe_softc_list_lock
2. aquire pppoe_softc lock
3. release pppoe_softc_list_lock
4. access to pppoe_softc
5. release pppoe_softc lock

However, pppoe_dispatch_disc_pkt() releases the lock of pppoe_softc
temporarily, and then re-aquires it before step 4 of the adove. So,
it is possible for other contexts to destroy a pppoe_softc in the
interim.
To fix this condition, avoid PPPOE_UNLOCK with the problem.

ok by knakahara@n.o

diffstat:

 sys/net/if_pppoe.c |  118 ++++++++++++++++++++++++++++------------------------
 1 files changed, 64 insertions(+), 54 deletions(-)

diffs (228 lines):

diff -r e0b90d1148ba -r 907cb28f04b7 sys/net/if_pppoe.c
--- a/sys/net/if_pppoe.c        Mon Jun 18 09:12:17 2018 +0000
+++ b/sys/net/if_pppoe.c        Mon Jun 18 09:49:05 2018 +0000
@@ -1,4 +1,4 @@
-/* $NetBSD: if_pppoe.c,v 1.138 2018/05/25 04:40:27 ozaki-r Exp $ */
+/* $NetBSD: if_pppoe.c,v 1.139 2018/06/18 09:49:05 yamaguchi Exp $ */
 
 /*-
  * Copyright (c) 2002, 2008 The NetBSD Foundation, Inc.
@@ -30,7 +30,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: if_pppoe.c,v 1.138 2018/05/25 04:40:27 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_pppoe.c,v 1.139 2018/06/18 09:49:05 yamaguchi Exp $");
 
 #ifdef _KERNEL_OPT
 #include "pppoe.h"
@@ -518,15 +518,15 @@
        size_t ac_cookie_len;
        uint8_t *relay_sid;
        size_t relay_sid_len;
-#ifdef PPPOE_SERVER
        uint8_t *hunique;
        size_t hunique_len;
-#endif
        struct pppoehdr *ph;
        struct pppoetag *pt;
        struct mbuf *n;
        int noff, err, errortag;
        struct ether_header *eh;
+       struct ifnet *rcvif;
+       struct psref psref;
 
        /* as long as we don't know which instance */
        strlcpy(devname, "pppoe", sizeof(devname));
@@ -545,10 +545,8 @@
        ac_cookie_len = 0;
        relay_sid = NULL;
        relay_sid_len = 0;
-#ifdef PPPOE_SERVER
        hunique = NULL;
        hunique_len = 0;
-#endif
        session = 0;
        if (m->m_pkthdr.len - off <= PPPOE_HEADERLEN) {
                printf("pppoe: packet too short: %d\n", m->m_pkthdr.len);
@@ -601,8 +599,7 @@
                case PPPOE_TAG_SNAME:
                        break;  /* ignored */
                case PPPOE_TAG_ACNAME:
-                       error = NULL;
-                       if (sc != NULL && len > 0) {
+                       if (len > 0) {
                                error = malloc(len + 1, M_TEMP, M_NOWAIT);
                                if (error == NULL)
                                        break;
@@ -616,40 +613,24 @@
                                }
 
                                strlcpy(error, mtod(n, char*) + noff, len + 1);
-                               printf("%s: connected to %s\n", devname, error);
+                               printf("pppoe: connected to %s\n", error);
                                free(error, M_TEMP);
                        }
                        break;  /* ignored */
-               case PPPOE_TAG_HUNIQUE: {
-                       struct ifnet *rcvif;
-                       struct psref psref;
+               case PPPOE_TAG_HUNIQUE:
+                       if (hunique == NULL) {
+                               n = m_pulldown(m, off + sizeof(*pt), len,
+                                   &noff);
+                               if (!n) {
+                                       m = NULL;
+                                       err_msg = "TAG HUNIQUE ERROR";
+                                       break;
+                               }
 
-                       if (sc != NULL)
-                               break;
-                       n = m_pulldown(m, off + sizeof(*pt), len, &noff);
-                       if (!n) {
-                               m = NULL;
-                               err_msg = "TAG HUNIQUE ERROR";
-                               break;
-                       }
-#ifdef PPPOE_SERVER
-                       hunique = mtod(n, uint8_t *) + noff;
-                       hunique_len = len;
-#endif
-                       rcvif = m_get_rcvif_psref(m, &psref);
-                       if (rcvif != NULL) {
-                               sc = pppoe_find_softc_by_hunique(
-                                       mtod(n, char *) + noff, len, rcvif,
-                                       RW_READER);
-                       }
-                       m_put_rcvif_psref(rcvif, &psref);
-                       if (sc != NULL) {
-                               strlcpy(devname, sc->sc_sppp.pp_if.if_xname,
-                                   sizeof(devname));
-                               PPPOE_UNLOCK(sc);
+                               hunique = mtod(n, uint8_t *) + noff;
+                               hunique_len = len;
                        }
                        break;
-               }
                case PPPOE_TAG_ACCOOKIE:
                        if (ac_cookie == NULL) {
                                n = m_pulldown(m, off + sizeof(*pt), len,
@@ -768,9 +749,6 @@
 #endif /* PPPOE_SERVER */
        case PPPOE_CODE_PADR:
 #ifdef PPPOE_SERVER
-           {
-               struct ifnet *rcvif;
-               struct psref psref;
                /*
                 * get sc from ac_cookie if IFF_PASSIVE
                 */
@@ -820,12 +798,24 @@
 
                sc->sc_sppp.pp_up(&sc->sc_sppp);
                break;
-           }
 #else
                /* ignore, we are no access concentrator */
                goto done;
 #endif /* PPPOE_SERVER */
        case PPPOE_CODE_PADO:
+               rcvif = m_get_rcvif_psref(m, &psref);
+               if (__predict_false(rcvif == NULL))
+                       goto done;
+
+               if (hunique != NULL) {
+                       sc = pppoe_find_softc_by_hunique(hunique,
+                                                        hunique_len,
+                                                        rcvif,
+                                                        RW_WRITER);
+               }
+
+               m_put_rcvif_psref(rcvif, &psref);
+
                if (sc == NULL) {
                        /* be quiet if there is not a single pppoe instance */
                        if (!LIST_EMPTY(&pppoe_softc_list))
@@ -833,8 +823,6 @@
                        goto done;
                }
 
-               PPPOE_LOCK(sc, RW_WRITER);
-
                if (sc->sc_state != PPPOE_STATE_PADI_SENT) {
                        printf("%s: received unexpected PADO\n",
                            sc->sc_sppp.pp_if.if_xname);
@@ -889,11 +877,22 @@
                PPPOE_UNLOCK(sc);
                break;
        case PPPOE_CODE_PADS:
+               rcvif = m_get_rcvif_psref(m, &psref);
+               if (__predict_false(rcvif == NULL))
+                       goto done;
+
+               if (hunique != NULL) {
+                       sc = pppoe_find_softc_by_hunique(hunique,
+                                                        hunique_len,
+                                                        rcvif,
+                                                        RW_WRITER);
+               }
+
+               m_put_rcvif_psref(rcvif, &psref);
+
                if (sc == NULL)
                        goto done;
 
-               PPPOE_LOCK(sc, RW_WRITER);
-
                sc->sc_session = session;
                callout_stop(&sc->sc_timeout);
                if (sc->sc_sppp.pp_if.if_flags & IFF_DEBUG)
@@ -904,26 +903,37 @@
 
                sc->sc_sppp.pp_up(&sc->sc_sppp);        /* notify upper layers */
                break;
-       case PPPOE_CODE_PADT: {
-               struct ifnet *rcvif;
-               struct psref psref;
-
+       case PPPOE_CODE_PADT:
                rcvif = m_get_rcvif_psref(m, &psref);
-               if (__predict_true(rcvif != NULL)) {
-                       sc = pppoe_find_softc_by_session(session, rcvif,
-                           RW_WRITER);
-               }
+               if (__predict_false(rcvif == NULL))
+                       goto done;
+
+               sc = pppoe_find_softc_by_session(session, rcvif,
+                   RW_WRITER);
+
                m_put_rcvif_psref(rcvif, &psref);
+
                if (sc == NULL)
                        goto done;
 
                pppoe_clear_softc(sc, "received PADT");
                PPPOE_UNLOCK(sc);
                break;
-       }
        default:
+               rcvif = m_get_rcvif_psref(m, &psref);
+               if (__predict_false(rcvif == NULL))
+                       goto done;
+
+               if (hunique != NULL) {
+                       sc = pppoe_find_softc_by_hunique(hunique,
+                                                        hunique_len,
+                                                        rcvif,
+                                                        RW_READER);
+               }
+
+               m_put_rcvif_psref(rcvif, &psref);
+
                if (sc != NULL) {
-                       PPPOE_LOCK(sc, RW_READER);
                        strlcpy(devname, sc->sc_sppp.pp_if.if_xname,
                            sizeof(devname));
                        PPPOE_UNLOCK(sc);



Home | Main Index | Thread Index | Old Index