Skip to content

Commit

Permalink
Fix possible double releasing for SA reference.
Browse files Browse the repository at this point in the history 
There are two possible ways how crypto callback are called: directly from
caller and deffered from crypto thread.

For inbound packets the direct call chain is the following:
 IPSEC_INPUT() method -> ipsec_common_input() -> xform_input() ->
 -> crypto_dispatch() -> crypto_invoke() -> crypto_done() ->
 -> xform_input_cb() -> ipsec[46]_common_input_cb() -> netisr_queue().

The SA reference is held while crypto processing is not finished.
The error handling code wrongly expected that crypto callback always called
from the crypto thread context, and it did SA reference releasing in
xform_input_cb(). But when the crypto callback called directly, in case of
error (e.g. data authentification failed) the error handling in
ipsec_common_input() also did SA reference releasing.

To fix this, remove error handling from ipsec_common_input() and do it
in xform_input() before crypto_dispatch().

PR:		219356
MFC after:	10 days


git-svn-id: svn+ssh://svn.freebsd.org/base/head@318734 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
  • Loading branch information
ae committed May 23, 2017
1 parent f53f262 commit 085a446
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 31 deletions.
2 changes: 0 additions & 2 deletions sys/netipsec/ipsec_input.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,6 @@ ipsec_common_input(struct mbuf *m, int skip, int protoff, int af, int sproto)
* everything else.
*/
error = (*sav->tdb_xform->xf_input)(m, sav, skip, protoff);
if (error != 0)
key_freesav(&sav);
return (error);
}

Expand Down
25 changes: 15 additions & 10 deletions sys/netipsec/xform_ah.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,8 +566,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
if (ah == NULL) {
DPRINTF(("ah_input: cannot pullup header\n"));
AHSTAT_INC(ahs_hdrops); /*XXX*/
m_freem(m);
return ENOBUFS;
error = ENOBUFS;
goto bad;
}

/* Check replay window, if applicable. */
Expand All @@ -578,8 +578,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
AHSTAT_INC(ahs_replay);
DPRINTF(("%s: packet replay failure: %s\n", __func__,
ipsec_sa2str(sav, buf, sizeof(buf))));
m_freem(m);
return (EACCES);
error = EACCES;
goto bad;
}
cryptoid = sav->tdb_cryptoid;
SECASVAR_UNLOCK(sav);
Expand All @@ -595,8 +595,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
ipsec_address(&sav->sah->saidx.dst, buf, sizeof(buf)),
(u_long) ntohl(sav->spi)));
AHSTAT_INC(ahs_badauthl);
m_freem(m);
return EACCES;
error = EACCES;
goto bad;
}
AHSTAT_ADD(ahs_ibytes, m->m_pkthdr.len - skip - hl);

Expand All @@ -606,8 +606,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
DPRINTF(("%s: failed to acquire crypto descriptor\n",
__func__));
AHSTAT_INC(ahs_crypto);
m_freem(m);
return ENOBUFS;
error = ENOBUFS;
goto bad;
}

crda = crp->crp_desc;
Expand All @@ -629,8 +629,8 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
DPRINTF(("%s: failed to allocate xform_data\n", __func__));
AHSTAT_INC(ahs_crypto);
crypto_freereq(crp);
m_freem(m);
return ENOBUFS;
error = ENOBUFS;
goto bad;
}

/*
Expand All @@ -650,6 +650,7 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
AHSTAT_INC(ahs_hdrops);
free(xd, M_XDATA);
crypto_freereq(crp);
key_freesav(&sav);
return (error);
}

Expand All @@ -668,6 +669,10 @@ ah_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
xd->skip = skip;
xd->cryptoid = cryptoid;
return (crypto_dispatch(crp));
bad:
m_freem(m);
key_freesav(&sav);
return (error);
}

/*
Expand Down
25 changes: 14 additions & 11 deletions sys/netipsec/xform_esp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,18 +272,18 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
struct newesp *esp;
uint8_t *ivp;
uint64_t cryptoid;
int plen, alen, hlen;
int alen, error, hlen, plen;

IPSEC_ASSERT(sav != NULL, ("null SA"));
IPSEC_ASSERT(sav->tdb_encalgxform != NULL, ("null encoding xform"));

error = EINVAL;
/* Valid IP Packet length ? */
if ( (skip&3) || (m->m_pkthdr.len&3) ){
DPRINTF(("%s: misaligned packet, skip %u pkt len %u",
__func__, skip, m->m_pkthdr.len));
ESPSTAT_INC(esps_badilen);
m_freem(m);
return EINVAL;
goto bad;
}
/* XXX don't pullup, just copy header */
IP6_EXTHDR_GET(esp, struct newesp *, m, skip, sizeof (struct newesp));
Expand Down Expand Up @@ -314,8 +314,7 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
ipsec_address(&sav->sah->saidx.dst, buf, sizeof(buf)),
(u_long)ntohl(sav->spi)));
ESPSTAT_INC(esps_badilen);
m_freem(m);
return EINVAL;
goto bad;
}

/*
Expand All @@ -328,8 +327,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
DPRINTF(("%s: packet replay check for %s\n", __func__,
ipsec_sa2str(sav, buf, sizeof(buf))));
ESPSTAT_INC(esps_replay);
m_freem(m);
return (EACCES);
error = EACCES;
goto bad;
}
}
cryptoid = sav->tdb_cryptoid;
Expand All @@ -344,8 +343,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
DPRINTF(("%s: failed to acquire crypto descriptors\n",
__func__));
ESPSTAT_INC(esps_crypto);
m_freem(m);
return ENOBUFS;
error = ENOBUFS;
goto bad;
}

/* Get IPsec-specific opaque pointer */
Expand All @@ -354,8 +353,8 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
DPRINTF(("%s: failed to allocate xform_data\n", __func__));
ESPSTAT_INC(esps_crypto);
crypto_freereq(crp);
m_freem(m);
return ENOBUFS;
error = ENOBUFS;
goto bad;
}

if (esph != NULL) {
Expand Down Expand Up @@ -425,6 +424,10 @@ esp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
crde->crd_alg = espx->type;

return (crypto_dispatch(crp));
bad:
m_freem(m);
key_freesav(&sav);
return (error);
}

/*
Expand Down
20 changes: 12 additions & 8 deletions sys/netipsec/xform_ipcomp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,43 +194,43 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
struct cryptop *crp;
struct ipcomp *ipcomp;
caddr_t addr;
int hlen = IPCOMP_HLENGTH;
int error, hlen = IPCOMP_HLENGTH;

/*
* Check that the next header of the IPComp is not IPComp again, before
* doing any real work. Given it is not possible to do double
* compression it means someone is playing tricks on us.
*/
error = ENOBUFS;
if (m->m_len < skip + hlen && (m = m_pullup(m, skip + hlen)) == NULL) {
IPCOMPSTAT_INC(ipcomps_hdrops); /*XXX*/
DPRINTF(("%s: m_pullup failed\n", __func__));
return (ENOBUFS);
key_freesav(&sav);
return (error);
}
addr = (caddr_t) mtod(m, struct ip *) + skip;
ipcomp = (struct ipcomp *)addr;
if (ipcomp->comp_nxt == IPPROTO_IPCOMP) {
m_freem(m);
IPCOMPSTAT_INC(ipcomps_pdrops); /* XXX have our own stats? */
DPRINTF(("%s: recursive compression detected\n", __func__));
return (EINVAL);
error = EINVAL;
goto bad;
}

/* Get crypto descriptors */
crp = crypto_getreq(1);
if (crp == NULL) {
m_freem(m);
DPRINTF(("%s: no crypto descriptors\n", __func__));
IPCOMPSTAT_INC(ipcomps_crypto);
return ENOBUFS;
goto bad;
}
/* Get IPsec-specific opaque pointer */
xd = malloc(sizeof(*xd), M_XDATA, M_NOWAIT | M_ZERO);
if (xd == NULL) {
DPRINTF(("%s: cannot allocate xform_data\n", __func__));
IPCOMPSTAT_INC(ipcomps_crypto);
crypto_freereq(crp);
m_freem(m);
return ENOBUFS;
goto bad;
}
crdc = crp->crp_desc;

Expand Down Expand Up @@ -259,6 +259,10 @@ ipcomp_input(struct mbuf *m, struct secasvar *sav, int skip, int protoff)
SECASVAR_UNLOCK(sav);

return crypto_dispatch(crp);
bad:
m_freem(m);
key_freesav(&sav);
return (error);
}

/*
Expand Down

0 comments on commit 085a446

Please sign in to comment.