diff mbox

[openggsn,v2] gtp: fix endianness in teid field of GTPv0 header

Message ID 1395400129-21471-1-git-send-email-pablo@gnumonks.org
State Accepted
Headers show

Commit Message

Pablo Neira Ayuso March 21, 2014, 11:08 a.m. UTC 
From: Pablo Neira Ayuso <pablo@gnumonks.org>

This field needs to be in network byte order as well. This problem
may manifest if you use ggsn and sgsn with different endianness.
It also show up when using the gtp kernel mode since the downlink
packets are using network byte order in the 64-bits teid field,
which osmo-sgsn / libgtp doesn't expect.
---
v2: Add function to encapsulate the code to access and to build
    the teid as Holger suggested.

Tested with and without gtp kernel support.
If no objections, I'll push this to master.

 gtp/gtp.c |   34 +++++++++++++---------------------
 gtp/pdp.c |    6 ++++++
 gtp/pdp.h |    2 ++
 3 files changed, 21 insertions(+), 21 deletions(-)

Comments

Holger Freyther March 23, 2014, 7:52 p.m. UTC | #1 
On Fri, Mar 21, 2014 at 12:08:49PM +0100, pablo@gnumonks.org wrote:
> From: Pablo Neira Ayuso <pablo@gnumonks.org>
> 
> This field needs to be in network byte order as well. This problem
> may manifest if you use ggsn and sgsn with different endianness.
> It also show up when using the gtp kernel mode since the downlink
> packets are using network byte order in the 64-bits teid field,
> which osmo-sgsn / libgtp doesn't expect.
> ---
> v2: Add function to encapsulate the code to access and to build
>     the teid as Holger suggested.

looks good.
diff mbox

Patch

diff --git a/gtp/gtp.c b/gtp/gtp.c
index 3cc0c0b..6185634 100644
--- a/gtp/gtp.c
+++ b/gtp/gtp.c
@@ -250,7 +250,7 @@  static uint64_t get_tid(void *pack)
 	union gtp_packet *packet = (union gtp_packet *)pack;
 
 	if ((packet->flags & 0xe0) == 0x00) {	/* Version 0 */
-		return packet->gtp0.h.tid;
+		return be64toh(packet->gtp0.h.tid);
 	}
 	return 0;
 }
@@ -425,10 +425,10 @@  int gtp_req(struct gsn_t *gsn, int version, struct pdp_t *pdp,
 		addr.sin_port = htons(GTP0_PORT);
 		packet->gtp0.h.length = hton16(len - GTP0_HEADER_SIZE);
 		packet->gtp0.h.seq = hton16(gsn->seq_next);
-		if (pdp)
+		if (pdp) {
 			packet->gtp0.h.tid =
-			    (pdp->imsi & 0x0fffffffffffffffull) +
-			    ((uint64_t) pdp->nsapi << 60);
+				htobe64(pdp_gettid(pdp->imsi, pdp->nsapi));
+		}
 		if (pdp && ((packet->gtp0.h.type == GTP_GPDU)
 			    || (packet->gtp0.h.type == GTP_ERROR)))
 			packet->gtp0.h.flow = hton16(pdp->flru);
@@ -581,7 +581,7 @@  int gtp_resp(int version, struct gsn_t *gsn, struct pdp_t *pdp,
 	if ((packet->flags & 0xe0) == 0x00) {	/* Version 0 */
 		packet->gtp0.h.length = hton16(len - GTP0_HEADER_SIZE);
 		packet->gtp0.h.seq = hton16(seq);
-		packet->gtp0.h.tid = tid;
+		packet->gtp0.h.tid = htobe64(tid);
 		if (pdp && ((packet->gtp0.h.type == GTP_GPDU) ||
 			    (packet->gtp0.h.type == GTP_ERROR)))
 			packet->gtp0.h.flow = hton16(pdp->flru);
@@ -1329,12 +1329,9 @@  int gtp_create_pdp_ind(struct gsn_t *gsn, int version,
 	memset(pdp, 0, sizeof(struct pdp_t));
 
 	if (version == 0) {
-		pdp->imsi =
-		    ((union gtp_packet *)pack)->gtp0.
-		    h.tid & 0x0fffffffffffffffull;
-		pdp->nsapi =
-		    (((union gtp_packet *)pack)->gtp0.
-		     h.tid & 0xf000000000000000ull) >> 60;
+		uint64_t tid = be64toh(((union gtp_packet *)pack)->gtp0.h.tid);
+
+		pdp_set_imsi_nsapi(pdp, tid);
 	}
 
 	pdp->seq = seq;
@@ -2051,12 +2048,9 @@  int gtp_update_pdp_ind(struct gsn_t *gsn, int version,
 	/* For GTP1 we must use imsi and nsapi if imsi is present. Otherwise */
 	/* we have to use the tunnel endpoint identifier */
 	if (version == 0) {
-		imsi =
-		    ((union gtp_packet *)pack)->gtp0.
-		    h.tid & 0x0fffffffffffffffull;
-		nsapi =
-		    (((union gtp_packet *)pack)->gtp0.
-		     h.tid & 0xf000000000000000ull) >> 60;
+		uint64_t tid = be64toh(((union gtp_packet *)pack)->gtp0.h.tid);
+
+		pdp_set_imsi_nsapi(pdp, tid);
 
 		/* Find the context in question */
 		if (pdp_getimsi(&pdp, imsi, nsapi)) {
@@ -2645,7 +2639,7 @@  int gtp_error_ind_conf(struct gsn_t *gsn, int version,
 	struct pdp_t *pdp;
 
 	/* Find the context in question */
-	if (pdp_tidget(&pdp, ((union gtp_packet *)pack)->gtp0.h.tid)) {
+	if (pdp_tidget(&pdp, be64toh(((union gtp_packet *)pack)->gtp0.h.tid))) {
 		gsn->err_unknownpdp++;
 		gtp_errpack(LOG_ERR, __FILE__, __LINE__, peer, pack, len,
 			    "Unknown PDP context");
@@ -3196,9 +3190,7 @@  int gtp_data_req(struct gsn_t *gsn, struct pdp_t *pdp, void *pack, unsigned len)
 		packet.gtp0.h.length = hton16(len);
 		packet.gtp0.h.seq = hton16(pdp->gtpsntx++);
 		packet.gtp0.h.flow = hton16(pdp->flru);
-		packet.gtp0.h.tid =
-		    (pdp->imsi & 0x0fffffffffffffffull) +
-		    ((uint64_t) pdp->nsapi << 60);
+		packet.gtp0.h.tid = htobe64(pdp_gettid(pdp->imsi, pdp->nsapi));
 
 		if (len > sizeof(union gtp_packet) - sizeof(struct gtp0_header)) {
 			gsn->err_memcpy++;
diff --git a/gtp/pdp.c b/gtp/pdp.c
index dfb91ea..dc21bf8 100644
--- a/gtp/pdp.c
+++ b/gtp/pdp.c
@@ -370,6 +370,12 @@  uint64_t pdp_gettid(uint64_t imsi, uint8_t nsapi)
 	return (imsi & 0x0fffffffffffffffull) + ((uint64_t) nsapi << 60);
 }
 
+void pdp_set_imsi_nsapi(struct pdp_t *pdp, uint64_t teid)
+{
+	pdp->imsi = teid & 0x0fffffffffffffffull;
+	pdp->nsapi = (teid & 0xf000000000000000ull) >> 60;
+}
+
 int ulcpy(void *dst, void *src, size_t size)
 {
 	if (((struct ul255_t *)src)->l <= size) {
diff --git a/gtp/pdp.h b/gtp/pdp.h
index b069a6f..6e30467 100644
--- a/gtp/pdp.h
+++ b/gtp/pdp.h
@@ -242,6 +242,8 @@  int pdp_tidset(struct pdp_t *pdp, uint64_t tid);
 int pdp_tiddel(struct pdp_t *pdp);
 int pdp_tidget(struct pdp_t **pdp, uint64_t tid);
 
+void pdp_set_imsi_nsapi(struct pdp_t *pdp, uint64_t teid);
+
 /*
 int pdp_iphash(void* ipif, struct ul66_t *eua);
 int pdp_ipset(struct pdp_t *pdp, void* ipif, struct ul66_t *eua);