diff mbox

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

Message ID 1395326040-6285-2-git-send-email-pablo@gnumonks.org
State Superseded
Headers show

Commit Message

Pablo Neira Ayuso March 20, 2014, 2:34 p.m. UTC 
From: Pablo Neira Ayuso <pablo@gnumonks.org>

This field needs to be in network byte order as well.
---
The problem only shows up if you use sgsn and ggsn with different
endianess. If no objections, I'll push this to master.

 gtp/gtp.c |   37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

Comments

Holger Freyther March 20, 2014, 9 p.m. UTC | #1 
On Thu, Mar 20, 2014 at 03:34:00PM +0100, pablo@gnumonks.org wrote:
> From: Pablo Neira Ayuso <pablo@gnumonks.org>
> 
> This field needs to be in network byte order as well.
> ---
> The problem only shows up if you use sgsn and ggsn with different
> endianess. If no objections, I'll push this to master.

Oh? Did you have a set-up where this actually happened? What do you
think about having macros to "write" and "read" the tid/imsi/nsapi?
Pablo Neira Ayuso March 21, 2014, 3:15 a.m. UTC | #2 
On Thu, Mar 20, 2014 at 10:00:54PM +0100, Holger Hans Peter Freyther wrote:
> On Thu, Mar 20, 2014 at 03:34:00PM +0100, pablo@gnumonks.org wrote:
> > From: Pablo Neira Ayuso <pablo@gnumonks.org>
> > 
> > This field needs to be in network byte order as well.
> > ---
> > The problem only shows up if you use sgsn and ggsn with different
> > endianess. If no objections, I'll push this to master.
> 
> Oh? Did you have a set-up where this actually happened? What do you
> think about having macros to "write" and "read" the tid/imsi/nsapi?

The problem also manifest with the gtp kernel module. Basically, the
uplink packets from osmo-ggsn doesn't convert teid to network byte
order. But downlink packets (going throught the gtp0 device) are
indeed using network byte order. I'll add this to the description as
well.

And OK, I'll add some static inline functions to wrap that code.

Thanks for the feedback.
diff mbox

Patch

diff --git a/gtp/gtp.c b/gtp/gtp.c
index 3cc0c0b..fd4f0d0 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,11 @@  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->imsi & 0x0fffffffffffffffull) +
+				    ((uint64_t) pdp->nsapi << 60));
+		}
 		if (pdp && ((packet->gtp0.h.type == GTP_GPDU)
 			    || (packet->gtp0.h.type == GTP_ERROR)))
 			packet->gtp0.h.flow = hton16(pdp->flru);
@@ -581,7 +582,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 +1330,10 @@  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->imsi = tid & 0x0fffffffffffffffull;
+		pdp->nsapi = (tid & 0xf000000000000000ull) >> 60;
 	}
 
 	pdp->seq = seq;
@@ -2051,12 +2050,10 @@  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);
+
+		imsi = tid & 0x0fffffffffffffffull;
+		nsapi = (tid & 0xf000000000000000ull) >> 60;
 
 		/* Find the context in question */
 		if (pdp_getimsi(&pdp, imsi, nsapi)) {
@@ -2645,7 +2642,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");
@@ -3197,8 +3194,8 @@  int gtp_data_req(struct gsn_t *gsn, struct pdp_t *pdp, void *pack, unsigned 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);
+		    htobe64((pdp->imsi & 0x0fffffffffffffffull) +
+			    ((uint64_t) pdp->nsapi << 60));
 
 		if (len > sizeof(union gtp_packet) - sizeof(struct gtp0_header)) {
 			gsn->err_memcpy++;