Module Name: src
Committed By: martin
Date: Thu Jul 12 15:11:56 UTC 2018
Modified Files:
src/sys/net [netbsd-8]: if_pppoe.c
Log Message:
Pull up following revision(s) (requested by yamaguchi in ticket #890):
sys/net/if_pppoe.c: revision 1.137
sys/net/if_pppoe.c: revision 1.139
sys/net/if_pppoe.c: revision 1.140
Drop early if there's no PPPoE interface. Otherwise it is easy for someone
to flood dmesg over the local subnet.
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 [email protected]
Fix to aquire pppoe_softc_list_lock before read and write the list
ok by [email protected]
To generate a diff of this commit:
cvs rdiff -u -r1.125.6.8 -r1.125.6.9 src/sys/net/if_pppoe.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/net/if_pppoe.c
diff -u src/sys/net/if_pppoe.c:1.125.6.8 src/sys/net/if_pppoe.c:1.125.6.9
--- src/sys/net/if_pppoe.c:1.125.6.8 Thu Jun 7 17:42:25 2018
+++ src/sys/net/if_pppoe.c Thu Jul 12 15:11:56 2018
@@ -1,4 +1,4 @@
-/* $NetBSD: if_pppoe.c,v 1.125.6.8 2018/06/07 17:42:25 martin Exp $ */
+/* $NetBSD: if_pppoe.c,v 1.125.6.9 2018/07/12 15:11:56 martin 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.125.6.8 2018/06/07 17:42:25 martin Exp $");
+__KERNEL_RCSID(0, "$NetBSD: if_pppoe.c,v 1.125.6.9 2018/07/12 15:11:56 martin Exp $");
#ifdef _KERNEL_OPT
#include "pppoe.h"
@@ -277,8 +277,11 @@ pppoedetach(void)
{
int error = 0;
- if (!LIST_EMPTY(&pppoe_softc_list))
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
+ if (!LIST_EMPTY(&pppoe_softc_list)) {
+ rw_exit(&pppoe_softc_list_lock);
error = EBUSY;
+ }
if (error == 0) {
if_clone_detach(&pppoe_cloner);
@@ -338,9 +341,12 @@ pppoe_clone_create(struct if_clone *ifc,
sppp_attach(&sc->sc_sppp.pp_if);
bpf_attach(&sc->sc_sppp.pp_if, DLT_PPP_ETHER, 0);
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
if (LIST_EMPTY(&pppoe_softc_list)) {
pfil_add_ihook(pppoe_ifattach_hook, NULL, PFIL_IFNET, if_pfil);
}
+ rw_exit(&pppoe_softc_list_lock);
+
if_register(&sc->sc_sppp.pp_if);
rw_init(&sc->sc_lock);
@@ -424,14 +430,18 @@ pppoe_find_softc_by_hunique(uint8_t *tok
{
struct pppoe_softc *sc, *t;
- if (LIST_EMPTY(&pppoe_softc_list))
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
+ if (LIST_EMPTY(&pppoe_softc_list)) {
+ rw_exit(&pppoe_softc_list_lock);
return NULL;
+ }
- if (len != sizeof sc)
+ if (len != sizeof sc) {
+ rw_exit(&pppoe_softc_list_lock);
return NULL;
+ }
memcpy(&t, token, len);
- rw_enter(&pppoe_softc_list_lock, RW_READER);
LIST_FOREACH(sc, &pppoe_softc_list, sc_list) {
if (sc == t) {
PPPOE_LOCK(sc, lock);
@@ -518,15 +528,15 @@ pppoe_dispatch_disc_pkt(struct mbuf *m,
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 +555,8 @@ pppoe_dispatch_disc_pkt(struct mbuf *m,
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 +609,7 @@ pppoe_dispatch_disc_pkt(struct mbuf *m,
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 +623,24 @@ pppoe_dispatch_disc_pkt(struct mbuf *m,
}
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,
@@ -722,9 +713,12 @@ breakbreak:;
* got service name, concentrator name, and/or host unique.
* ignore if we have no interfaces with IFF_PASSIVE|IFF_UP.
*/
- if (LIST_EMPTY(&pppoe_softc_list))
- goto done;
rw_enter(&pppoe_softc_list_lock, RW_READER);
+ if (LIST_EMPTY(&pppoe_softc_list)) {
+ rw_exit(&pppoe_softc_list_lock);
+ goto done;
+ }
+
LIST_FOREACH(sc, &pppoe_softc_list, sc_list) {
PPPOE_LOCK(sc, RW_WRITER);
if (!(sc->sc_sppp.pp_if.if_flags & IFF_UP)) {
@@ -768,9 +762,6 @@ breakbreak:;
#endif /* PPPOE_SERVER */
case PPPOE_CODE_PADR:
#ifdef PPPOE_SERVER
- {
- struct ifnet *rcvif;
- struct psref psref;
/*
* get sc from ac_cookie if IFF_PASSIVE
*/
@@ -790,8 +781,12 @@ breakbreak:;
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))
- printf("pppoe: received PADR but could not find request for it\n");
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
+ if (!LIST_EMPTY(&pppoe_softc_list)) {
+ printf("pppoe: received PADR"
+ " but could not find request for it\n");
+ }
+ rw_exit(&pppoe_softc_list_lock);
goto done;
}
@@ -820,21 +815,35 @@ breakbreak:;
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))
- printf("pppoe: received PADO but could not find request for it\n");
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
+ if (!LIST_EMPTY(&pppoe_softc_list)) {
+ printf("pppoe: received PADO"
+ " but could not find request for it\n");
+ }
+ rw_exit(&pppoe_softc_list_lock);
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,10 +898,21 @@ breakbreak:;
PPPOE_UNLOCK(sc);
break;
case PPPOE_CODE_PADS:
- if (sc == NULL)
+ rcvif = m_get_rcvif_psref(m, &psref);
+ if (__predict_false(rcvif == NULL))
goto done;
- PPPOE_LOCK(sc, RW_WRITER);
+ 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;
sc->sc_session = session;
callout_stop(&sc->sc_timeout);
@@ -904,26 +924,37 @@ breakbreak:;
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);
@@ -944,13 +975,19 @@ done:
static void
pppoe_disc_input(struct mbuf *m)
{
+ KASSERT(m->m_flags & M_PKTHDR);
- /* avoid error messages if there is not a single pppoe instance */
+ /*
+ * Avoid error messages if there is not a single PPPoE instance.
+ */
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
if (!LIST_EMPTY(&pppoe_softc_list)) {
- KASSERT(m->m_flags & M_PKTHDR);
+ rw_exit(&pppoe_softc_list_lock);
pppoe_dispatch_disc_pkt(m, 0);
- } else
+ } else {
+ rw_exit(&pppoe_softc_list_lock);
m_freem(m);
+ }
}
static bool
@@ -977,6 +1014,16 @@ pppoe_data_input(struct mbuf *m)
KASSERT(m->m_flags & M_PKTHDR);
+ /*
+ * Avoid error messages if there is not a single PPPoE instance.
+ */
+ rw_enter(&pppoe_softc_list_lock, RW_READER);
+ if (LIST_EMPTY(&pppoe_softc_list)) {
+ rw_exit(&pppoe_softc_list_lock);
+ goto drop;
+ }
+ rw_exit(&pppoe_softc_list_lock);
+
if (term_unknown) {
memcpy(shost, mtod(m, struct ether_header*)->ether_shost,
ETHER_ADDR_LEN);
