DF-0605 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 | diff --git a/sys/net/pf/pf_if.c b/sys/net/pf/pf_if.c --- a/sys/net/pf/pf_if.c +++ b/sys/net/pf/pf_if.c @@ -283,6 +283,17 @@ if (ifp->if_dunit == IF_DUNIT_NONE) return; + /* + * Serialize tree mutation with the unlocked RB walk in + * pfi_get_ifaces()/pfi_set_flags()/pfi_clear_flags(), which run + * under pf_token (held by pfioctl). Without this, an + * ifnet_attach/detach_event firing on a remote CPU can + * RB_INSERT/RB_REMOVE/free a kif that the walker has already + * captured in its nextp pointer, causing a use-after-free read + * (and potentially a panic when the stale pointer is later + * dereferenced). See DF-0605. + */ + lwkt_gettoken(&pf_token); crit_enter(); pfi_update++; if ((kif = pfi_kif_get(ifp->if_xname)) == NULL) @@ -291,6 +302,7 @@ ifp->if_pf_kif = kif; pfi_kif_update(kif); crit_exit(); + lwkt_reltoken(&pf_token); } void @@ -301,6 +313,8 @@ if ((kif = (struct pfi_kif *)ifp->if_pf_kif) == NULL) return; + /* See comment in pfi_attach_ifnet(). */ + lwkt_gettoken(&pf_token); crit_enter(); pfi_update++; pfi_kif_update(kif); @@ -308,6 +322,7 @@ ifp->if_pf_kif = NULL; pfi_kif_unref(kif, PFI_KIF_REF_NONE); crit_exit(); + lwkt_reltoken(&pf_token); } void @@ -315,6 +330,8 @@ { struct pfi_kif *kif; + /* See comment in pfi_attach_ifnet(). */ + lwkt_gettoken(&pf_token); crit_enter(); pfi_update++; if ((kif = pfi_kif_get(ifg->ifg_group)) == NULL) @@ -322,6 +339,7 @@ kif->pfik_group = ifg; ifg->ifg_pf_kif = kif; crit_exit(); + lwkt_reltoken(&pf_token); } void @@ -332,12 +350,15 @@ if ((kif = (struct pfi_kif *)ifg->ifg_pf_kif) == NULL) return; + /* See comment in pfi_attach_ifnet(). */ + lwkt_gettoken(&pf_token); crit_enter(); pfi_update++; kif->pfik_group = NULL; ifg->ifg_pf_kif = NULL; pfi_kif_unref(kif, PFI_KIF_REF_NONE); crit_exit(); + lwkt_reltoken(&pf_token); } void @@ -345,12 +366,15 @@ { struct pfi_kif *kif; + /* See comment in pfi_attach_ifnet(). */ + lwkt_gettoken(&pf_token); crit_enter(); pfi_update++; if ((kif = pfi_kif_get(group)) == NULL) panic("%s: pfi_kif_get failed", __func__); pfi_kif_update(kif); crit_exit(); + lwkt_reltoken(&pf_token); } int @@ -766,6 +790,16 @@ struct pfi_kif *p, *nextp; int n = 0; + /* + * Hold pf_token across the RB walk so concurrent + * pfi_attach_ifnet/pfi_detach_ifnet/pfi_attach_ifgroup/ + * pfi_detach_ifgroup/pfi_group_change event handlers (which also + * take pf_token) cannot RB_INSERT/RB_REMOVE/free the kif that + * 'nextp' already points at. Recursive-safe: pfioctl() already + * holds pf_token around DIOCIGETIFACES, but the lock is needed + * here in case a future caller forgets. See DF-0605. + */ + lwkt_gettoken(&pf_token); crit_enter(); for (p = RB_MIN(pfi_ifhead, &pfi_ifs); p; p = nextp) { nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p); @@ -778,6 +812,7 @@ if (copyout(p, buf++, sizeof(*buf))) { pfi_kif_unref(p, PFI_KIF_REF_RULE); crit_exit(); + lwkt_reltoken(&pf_token); return (EFAULT); } nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p); @@ -785,6 +820,7 @@ } } crit_exit(); + lwkt_reltoken(&pf_token); *size = n; return (0); } @@ -822,6 +858,8 @@ { struct pfi_kif *p; + /* See DF-0605: serialize RB walk with concurrent tree mutators. */ + lwkt_gettoken(&pf_token); crit_enter(); RB_FOREACH(p, pfi_ifhead, &pfi_ifs) { if (pfi_skip_if(name, p)) @@ -829,6 +867,7 @@ p->pfik_flags |= flags; } crit_exit(); + lwkt_reltoken(&pf_token); return (0); } @@ -837,6 +876,8 @@ { struct pfi_kif *p; + /* See DF-0605: serialize RB walk with concurrent tree mutators. */ + lwkt_gettoken(&pf_token); crit_enter(); RB_FOREACH(p, pfi_ifhead, &pfi_ifs) { if (pfi_skip_if(name, p)) @@ -844,6 +885,7 @@ p->pfik_flags &= ~flags; } crit_exit(); + lwkt_reltoken(&pf_token); return (0); } |