[PATCH 1/2 openggsn] gtp: fix wrong binary layout for struct gtp0_header in x86_64
From: Pablo Neira Ayuso <pablo@gnumonks.org> struct gtp0_header needs __attribute__((packed)) to make sure that gcc doesn't add a hole of 4 bytes to align the 64-bits teid, resulting in 24 bytes instead of 20 bytes. This was breaking gtpv0 in my gprs testbed with my x86_64 laptop. While at it, add also attribute packed to other headers just to make sure that gcc doesn't pad the structures with holes. --- If no objections, I'll push this to master. gtp/gtp.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gtp/gtp.h b/gtp/gtp.h index 54af96e..39a902f 100644 --- a/gtp/gtp.h +++ b/gtp/gtp.h @@ -161,7 +161,7 @@ struct gtp0_header { /* Descriptions from 3GPP 09.60 */ uint8_t spare2; /* 11 Spare */ uint8_t spare3; /* 12 Spare */ uint64_t tid; /* 13 Tunnel ID */ -}; /* 20 */ +} __attribute__((packed)); /* 20 */ struct gtp1_header_short { /* Descriptions from 3GPP 29060 */ uint8_t flags; /* 01 bitfield, with typical values */ @@ -174,7 +174,7 @@ struct gtp1_header_short { /* Descriptions from 3GPP 29060 */ uint8_t type; /* 02 Message type. T-PDU = 0xff */ uint16_t length; /* 03 Length (of IP packet or signalling) */ uint32_t tei; /* 05 - 08 Tunnel Endpoint ID */ -}; +} __attribute__((packed)); struct gtp1_header_long { /* Descriptions from 3GPP 29060 */ uint8_t flags; /* 01 bitfield, with typical values */ @@ -190,7 +190,7 @@ struct gtp1_header_long { /* Descriptions from 3GPP 29060 */ uint16_t seq; /* 10 Sequence Number */ uint8_t npdu; /* 11 N-PDU Number */ uint8_t next; /* 12 Next extension header type. Empty = 0 */ -}; +} __attribute__((packed)); struct gtp0_packet { struct gtp0_header h; -- 1.7.10.4
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(-) 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++; -- 1.7.10.4
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?
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.
participants (3)
-
Holger Hans Peter Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org