dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-sgsn/+/43958?usp=email )
Change subject: gprs_sndcp_xid: replace OSMO_ASSERT with if and return ......................................................................
gprs_sndcp_xid: replace OSMO_ASSERT with if and return
The code around the function gprs_sndcp_compile_xid was written under the assumption that the caller always supplies correct input parameters because the caller has 100% control over the input. However, this is a high standard and depending on how the input is obtained errors may happen. In case of an error, the whole process will crash due to the usage of OSMO_ASSERT.
When looking at the callers of gprs_sndcp_compile_xid, we can see that they already check the return code. Also the return type of gprs_sndcp_compile_xid is already int. We also do not have any dynamically allocated memory. There is nothing that would speak against returning a negative error code.
Let's replace each occurrence of OSMO_ASSERT with an if and return an error code to the caller, so that the caller can handle the problem gracefully.
Related: OS#7095 Change-Id: Ia570b27a3670fed3aeb4779840292c5d216a1102 --- M src/sgsn/gprs_sndcp_xid.c 1 file changed, 142 insertions(+), 77 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-sgsn refs/changes/58/43958/1
diff --git a/src/sgsn/gprs_sndcp_xid.c b/src/sgsn/gprs_sndcp_xid.c index 834d98a..64d8acf 100644 --- a/src/sgsn/gprs_sndcp_xid.c +++ b/src/sgsn/gprs_sndcp_xid.c @@ -49,20 +49,24 @@ /* FUNCTIONS RELATED TO SNDCP-XID ENCODING */
/* Encode applicable sapis (works the same in all three compression schemes) */ -static int encode_pcomp_applicable_sapis(uint8_t *dst, +static int encode_pcomp_applicable_sapis(uint8_t *dst, unsigned int dst_maxlen, const uint8_t *nsapis, uint8_t nsapis_len) { - /* NOTE: Buffer *dst needs offer at 2 bytes + /* NOTE: Buffer *dst needs offer at least 2 bytes * of space to store the generation results */
uint16_t blob; uint8_t nsapi; int i;
+ if (dst_maxlen < 2) + return -ENOMEM; + /* Bail if number of possible nsapis exceeds valid range * (Only 11 nsapis possible for PDP-Contexts) */ - OSMO_ASSERT(nsapis_len <= 11); + if (nsapis_len > 11) + return -EINVAL;
/* Encode applicable SAPIs */ blob = 0; @@ -71,7 +75,9 @@ /* Only NSAPI 5 to 15 are applicable for user traffic (PDP- * contexts). Only for these NSAPIs SNDCP-XID parameters * can apply. See also 3GPP TS 44.065, 5.1 Service primitives */ - OSMO_ASSERT(nsapi >= 5 && nsapi <= 15); + if (nsapi < 5 || nsapi > 15) + return -EINVAL; + blob |= (1 << nsapi); }
@@ -95,20 +101,25 @@ int dst_counter = 0; int rc;
- OSMO_ASSERT(dst_maxlen >= 3); + if (dst_maxlen < 3) + return -ENOMEM;
/* Zero out buffer */ memset(dst, 0, dst_maxlen);
/* Encode applicable SAPIs */ - rc = encode_pcomp_applicable_sapis(dst, params->nsapi, + rc = encode_pcomp_applicable_sapis(dst, dst_maxlen, params->nsapi, params->nsapi_len); + if (rc < 0) + return rc; dst += rc; dst_counter += rc;
/* Encode s01 (see also: 3GPP TS 44.065, 6.5.2.1, Table 5) */ - OSMO_ASSERT(params->s01 >= 0); - OSMO_ASSERT(params->s01 <= 255); + if (params->s01 < 0) + return -EINVAL; + if (params->s01 > 255) + return -EINVAL; *dst = params->s01; dst++; dst_counter++; @@ -125,26 +136,31 @@ const struct gprs_sndcp_pcomp_rfc2507_params *params) { - /* NOTE: Buffer *dst should offer at least 3 bytes + /* NOTE: Buffer *dst should offer at least 9 bytes * of space to store the generation results */
int dst_counter = 0; int rc;
- OSMO_ASSERT(dst_maxlen >= 9); + if (dst_maxlen < 9) + return -ENOMEM;
/* Zero out buffer */ memset(dst, 0, dst_maxlen);
/* Encode applicable SAPIs */ - rc = encode_pcomp_applicable_sapis(dst, params->nsapi, + rc = encode_pcomp_applicable_sapis(dst, dst_maxlen, params->nsapi, params->nsapi_len); + if (rc < 0) + return rc; dst += rc; dst_counter += rc;
/* Encode F_MAX_PERIOD (see also: 3GPP TS 44.065, 6.5.3.1, Table 6) */ - OSMO_ASSERT(params->f_max_period >= 1); - OSMO_ASSERT(params->f_max_period <= 65535); + if (params->f_max_period < 1) + return -EINVAL; + if (params->f_max_period > 65535) + return -EINVAL; *dst = (params->f_max_period >> 8) & 0xFF; dst++; dst_counter++; @@ -153,29 +169,38 @@ dst_counter++;
/* Encode F_MAX_TIME (see also: 3GPP TS 44.065, 6.5.3.1, Table 6) */ - OSMO_ASSERT(params->f_max_time >= 1); - OSMO_ASSERT(params->f_max_time <= 255); + if (params->f_max_time < 1) + return -EINVAL; + if (params->f_max_time > 255) + return -EINVAL; + *dst = params->f_max_time; dst++; dst_counter++;
/* Encode MAX_HEADER (see also: 3GPP TS 44.065, 6.5.3.1, Table 6) */ - OSMO_ASSERT(params->max_header >= 60); - OSMO_ASSERT(params->max_header <= 255); + if (params->max_header < 60) + return -EINVAL; + if (params->max_header > 255) + return -EINVAL; *dst = params->max_header; dst++; dst_counter++;
/* Encode TCP_SPACE (see also: 3GPP TS 44.065, 6.5.3.1, Table 6) */ - OSMO_ASSERT(params->tcp_space >= 3); - OSMO_ASSERT(params->tcp_space <= 255); + if (params->tcp_space < 3) + return -EINVAL; + if (params->tcp_space > 255) + return -EINVAL; *dst = params->tcp_space; dst++; dst_counter++;
/* Encode NON_TCP_SPACE (see also: 3GPP TS 44.065, 6.5.3.1, Table 6) */ - OSMO_ASSERT(params->non_tcp_space >= 3); - OSMO_ASSERT(params->non_tcp_space <= 65535); + if (params->non_tcp_space < 3) + return -EINVAL; + if (params->non_tcp_space > 65535) + return -EINVAL; *dst = (params->non_tcp_space >> 8) & 0xFF; dst++; dst_counter++; @@ -193,7 +218,7 @@ const struct gprs_sndcp_pcomp_rohc_params *params) { - /* NOTE: Buffer *dst should offer at least 36 + /* NOTE: Buffer *dst should offer at least 38 * (2 * 16 Profiles + 2 * 3 Parameter) bytes * of memory space to store generation results */
@@ -201,24 +226,30 @@ int dst_counter = 0; int rc;
- OSMO_ASSERT(dst_maxlen >= 38); + if (dst_maxlen < 38) + return -ENOMEM;
/* Bail if number of ROHC profiles exceeds limit * (ROHC supports only a maximum of 16 different profiles) */ - OSMO_ASSERT(params->profile_len <= 16); + if (params->profile_len > 16) + return -EINVAL;
/* Zero out buffer */ memset(dst, 0, dst_maxlen);
/* Encode applicable SAPIs */ - rc = encode_pcomp_applicable_sapis(dst, params->nsapi, + rc = encode_pcomp_applicable_sapis(dst, dst_maxlen, params->nsapi, params->nsapi_len); + if (rc < 0) + return rc; dst += rc; dst_counter += rc;
/* Encode MAX_CID (see also: 3GPP TS 44.065, 6.5.4.1, Table 10) */ - OSMO_ASSERT(params->max_cid >= 0); - OSMO_ASSERT(params->max_cid <= 16383); + if (params->max_cid < 0) + return -EINVAL; + if (params->max_cid > 16383) + return -EINVAL; *dst = (params->max_cid >> 8) & 0xFF; dst++; *dst = params->max_cid & 0xFF; @@ -226,8 +257,10 @@ dst_counter += 2;
/* Encode MAX_HEADER (see also: 3GPP TS 44.065, 6.5.4.1, Table 10) */ - OSMO_ASSERT(params->max_header >= 60); - OSMO_ASSERT(params->max_header <= 255); + if (params->max_header < 60) + return -EINVAL; + if (params->max_header > 255) + return -EINVAL; *dst = (params->max_header >> 8) & 0xFF; dst++; *dst = params->max_header & 0xFF; @@ -259,27 +292,34 @@ int dst_counter = 0; int rc;
- OSMO_ASSERT(dst_maxlen >= 6); + if (dst_maxlen < 6) + return -ENOMEM;
/* Zero out buffer */ memset(dst, 0, dst_maxlen);
/* Encode applicable SAPIs */ - rc = encode_pcomp_applicable_sapis(dst, params->nsapi, + rc = encode_pcomp_applicable_sapis(dst, dst_maxlen, params->nsapi, params->nsapi_len); + if (rc < 0) + return rc; dst += rc; dst_counter += rc;
/* Encode P0 (see also: 3GPP TS 44.065, 6.6.2.1, Table 7a) */ - OSMO_ASSERT(params->p0 >= 0); - OSMO_ASSERT(params->p0 <= 3); + if (params->p0 < 0) + return -EINVAL; + if (params->p0 > 3) + return -EINVAL; *dst = params->p0 & 0x03; dst++; dst_counter++;
/* Encode P1 (see also: 3GPP TS 44.065, 6.6.2.1, Table 7a) */ - OSMO_ASSERT(params->p1 >= 512); - OSMO_ASSERT(params->p1 <= 65535); + if (params->p1 < 512) + return -EINVAL; + if (params->p1 > 65535) + return -EINVAL; *dst = (params->p1 >> 8) & 0xFF; dst++; *dst = params->p1 & 0xFF; @@ -287,8 +327,10 @@ dst_counter += 2;
/* Encode P2 (see also: 3GPP TS 44.065, 6.6.2.1, Table 7a) */ - OSMO_ASSERT(params->p2 >= 6); - OSMO_ASSERT(params->p2 <= 250); + if (params->p2 < 6) + return -EINVAL; + if (params->p2 > 250) + return -EINVAL; *dst = params->p2; dst++; dst_counter++; @@ -309,33 +351,41 @@ int dst_counter = 0; int rc;
- OSMO_ASSERT(dst_maxlen >= 12); + if (dst_maxlen < 12) + return -ENOMEM;
/* Zero out buffer */ memset(dst, 0, dst_maxlen);
/* Encode applicable SAPIs */ - rc = encode_pcomp_applicable_sapis(dst, params->nsapi, + rc = encode_pcomp_applicable_sapis(dst, dst_maxlen, params->nsapi, params->nsapi_len); + if (rc < 0) + return rc; dst += rc; dst_counter += rc;
/* Encode C0 (see also: 3GPP TS 44.065, 6.6.3.1, Table 7c) */ - OSMO_ASSERT(params->c0 == 0x80 || params->c0 == 0xC0); + if (params->c0 != 0x80 && params->c0 != 0xC0) + return -EINVAL; *dst = params->c0 & 0xC0; dst++; dst_counter++;
/* Encode P0 (see also: 3GPP TS 44.065, 6.6.3.1, Table 7c) */ - OSMO_ASSERT(params->p0 >= 0); - OSMO_ASSERT(params->p0 <= 3); + if (params->p0 < 0) + return -EINVAL; + if (params->p0 > 3) + return -EINVAL; *dst = params->p0 & 0x03; dst++; dst_counter++;
/* Encode P1T (see also: 3GPP TS 44.065, 6.6.3.1, Table 7c) */ - OSMO_ASSERT(params->p1t >= 256); - OSMO_ASSERT(params->p1t <= 65535); + if (params->p1t < 256) + return -EINVAL; + if (params->p1t > 65535) + return -EINVAL; *dst = (params->p1t >> 8) & 0xFF; dst++; *dst = params->p1t & 0xFF; @@ -343,8 +393,10 @@ dst_counter += 2;
/* Encode P1R (see also: 3GPP TS 44.065, 6.6.3.1, Table 7c) */ - OSMO_ASSERT(params->p1r >= 256); - OSMO_ASSERT(params->p1r <= 65535); + if (params->p1r < 256) + return -EINVAL; + if (params->p1r > 65535) + return -EINVAL; *dst = (params->p1r >> 8) & 0xFF; dst++; *dst = params->p1r & 0xFF; @@ -352,9 +404,12 @@ dst_counter += 2;
/* Encode P3T (see also: 3GPP TS 44.065, 6.6.3.1, Table 7c) */ - OSMO_ASSERT(params->p3t >= 0); - OSMO_ASSERT(params->p3t <= 65535); - OSMO_ASSERT(params->p3t >= 2 * params->p1t); + if (params->p3t < 0) + return -EINVAL; + if (params->p3t > 65535) + return -EINVAL; + if (params->p3t < 2 * params->p1t) + return -EINVAL; *dst = (params->p3t >> 8) & 0xFF; dst++; *dst = params->p3t & 0xFF; @@ -362,9 +417,12 @@ dst_counter += 2;
/* Encode P3R (see also: 3GPP TS 44.065, 6.6.3.1, Table 7c) */ - OSMO_ASSERT(params->p3r >= 0); - OSMO_ASSERT(params->p3r <= 65535); - OSMO_ASSERT(params->p3r >= 2 * params->p1r); + if (params->p3r < 0) + return -EINVAL; + if (params->p3r > 65535) + return -EINVAL; + if (params->p3r < 2 * params->p1r) + return -EINVAL; *dst = (params->p3r >> 8) & 0xFF; dst++; *dst = params->p3r & 0xFF; @@ -390,6 +448,10 @@ uint8_t payload_bytes[256]; int payload_bytes_len = -1;
+ /* Bail if comp_len is out of bounds */ + if (comp_field->comp_len > sizeof(comp_field->comp)) + return -EINVAL; + /* If possible, try do encode payload bytes first */ if (comp_field->rfc1144_params) { payload_bytes_len = @@ -417,13 +479,11 @@ sizeof(payload_bytes), comp_field->v44_params); } else - OSMO_ASSERT(false); + return -EINVAL;
- /* Bail immediately if payload byte generation failed */ - OSMO_ASSERT(payload_bytes_len >= 0); - - /* Bail if comp_len is out of bounds */ - OSMO_ASSERT(comp_field->comp_len <= sizeof(comp_field->comp)); + /* Stop immediately if payload byte generation has failed */ + if (payload_bytes_len < 0) + return payload_bytes_len;
/* Calculate length field of the data block */ if (comp_field->p) { @@ -436,23 +496,29 @@ expected_length = len + 2; }
- /* Bail immediately if no sufficient memory space is supplied */ - OSMO_ASSERT(dst_maxlen >= expected_length); + /* Stop immediately if no sufficient memory space is supplied */ + if (expected_length > dst_maxlen) + return -ENOMEM;
/* Check if the entity number is within bounds */ - OSMO_ASSERT(comp_field->entity <= 0x1f); + if (comp_field->entity > 0x1f) + return -EINVAL;
/* Check if the algorithm number is within bounds */ compclass = gprs_sndcp_get_compression_class(comp_field); switch (compclass) { case SNDCP_XID_PROTOCOL_COMPRESSION: - OSMO_ASSERT(comp_field->algo.pcomp >= 0 && comp_field->algo.pcomp <= 0x1f); + if (comp_field->algo.pcomp < 0 || comp_field->algo.pcomp > 0x1f) + return -EINVAL; + if (comp_field->algo.pcomp < 0 || comp_field->algo.pcomp > 0x1f) + return -EINVAL; break; case SNDCP_XID_DATA_COMPRESSION: - OSMO_ASSERT(comp_field->algo.dcomp >= 0 && comp_field->algo.dcomp <= 0x1f); + if (comp_field->algo.dcomp < 0 || comp_field->algo.dcomp > 0x1f) + return -EINVAL; break; default: - OSMO_ASSERT(false); + return -EINVAL; }
/* Zero out buffer */ @@ -547,10 +613,8 @@ rc = encode_comp_field(dst + byte_counter, dst_maxlen - byte_counter, comp_field); - - /* When input data is correct, there is - * no reason for the encoder to fail! */ - OSMO_ASSERT(rc >= 0); + if (rc < 0) + return rc;
byte_counter += rc; } @@ -571,18 +635,19 @@
OSMO_ASSERT(comp_fields); OSMO_ASSERT(dst); - OSMO_ASSERT(dst_maxlen >= 2 + sizeof(xid_version_number));
/* Prepend header with version number */ if (version >= 0) { + byte_counter += TLV_GROSS_LEN(sizeof(xid_version_number)); + if (byte_counter > dst_maxlen) + return -ENOMEM; xid_version_number[0] = (uint8_t) (version & 0xff); dst = tlv_put(dst, SNDCP_XID_VERSION_NUMBER, sizeof(xid_version_number), xid_version_number); - byte_counter += (sizeof(xid_version_number) + 2); }
- /* Stop if there is no compression fields supplied */ + /* Bail if there is no compression fields supplied */ if (llist_empty(comp_fields)) return byte_counter;
@@ -590,9 +655,9 @@ rc = gprs_sndcp_pack_fields(comp_fields, comp_bytes, sizeof(comp_bytes), SNDCP_XID_DATA_COMPRESSION); - OSMO_ASSERT(rc >= 0); - - if (rc > 0) { + if (rc < 0) { + return rc; + } else if (rc > 0) { byte_counter += TLV_GROSS_LEN(rc); if (byte_counter > dst_maxlen) return -ENOMEM; @@ -603,9 +668,9 @@ rc = gprs_sndcp_pack_fields(comp_fields, comp_bytes, sizeof(comp_bytes), SNDCP_XID_PROTOCOL_COMPRESSION); - OSMO_ASSERT(rc >= 0); - - if (rc > 0) { + if (rc < 0) { + return rc; + } else if (rc > 0) { byte_counter += TLV_GROSS_LEN(rc); if (byte_counter > dst_maxlen) return -ENOMEM;