[MERGED] osmo-bts[master]: l1sap: Validate incoming RTP payload, drop bw-efficient AMR
Harald Welte has submitted this change and it was merged. Change subject: l1sap: Validate incoming RTP payload, drop bw-efficient AMR .. l1sap: Validate incoming RTP payload, drop bw-efficient AMR A recurrent kernel crash in sysmobts (several kernel versions) corrupting kernel memory in random places has been investigated and reproduced by placing a call against an MSC sending RTP with bandwidth-efficient AMR payload to osmo-bts-sysmo. The osmo-bts-sysmo in turn sends the payload to the femtobts related kernel modules via a msgq, which most probably fail to handle correctly this bw-efficient AMR payload and corrupt the kernel memory. First approach was to drop the bw-efficient AMR payloads lower in the stack in sysmo specific code (l1if_tch_encode), but as there's no bts model in osmo-bts actually supporting bw-efficient AMR, let's drop it early in the incoming path for all models to avoid further problems. Related: SYS#4063 Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 --- M src/common/l1sap.c 1 file changed, 45 insertions(+), 0 deletions(-) Approvals: Pau Espin Pedrol: Looks good to me, but someone else must approve; Verified Harald Welte: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/common/l1sap.c b/src/common/l1sap.c index e181a73..1deee83 100644 --- a/src/common/l1sap.c +++ b/src/common/l1sap.c @@ -819,6 +819,47 @@ return 0; } +static bool rtppayload_is_octet_aligned(const uint8_t *rtp_pl, uint8_t payload_len) +{ + /* +* Logic: If 1st bit padding is not zero, packet is either: +* - bandwidth-efficient AMR payload. +* - malformed packet. +* However, Bandwidth-efficient AMR 4,75 frame last in payload(F=0, FT=0) +* with 4th,5ht,6th AMR payload to 0 matches padding==0. +* Furthermore, both AMR 4,75 bw-efficient and octet alignment are 14 bytes long (AMR 4,75 encodes 95b): +* bw-efficient: 95b, + 4b hdr + 6b ToC = 105b, + padding = 112b = 14B. +* octet-aligned: 1B hdr + 1B ToC + 95b = 111b, + padding = 112b = 14B. +* We cannot use other fields to match since they are inside the AMR +* payload bits which are unknown. +* As a result, this function may return false positive (true) for some AMR +* 4,75 AMR frames, but given the length, CMR and FT read is the same as a +* consequence, the damage in here is harmless other than being unable to +* decode the audio at the other side. +*/ + #define AMR_PADDING1(rtp_pl) (rtp_pl[0] & 0x0f) + #define AMR_PADDING2(rtp_pl) (rtp_pl[1] & 0x03) + + if(payload_len < 2 || AMR_PADDING1(rtp_pl) || AMR_PADDING2(rtp_pl)) + return false; + + return true; +} + +static bool rtppayload_is_valid(struct gsm_lchan *lchan, struct msgb *resp_msg) +{ + /* Avoid sending bw-efficient AMR to lower layers, most bts models +* don't support it. */ + if(lchan->tch_mode == GSM48_CMODE_SPEECH_AMR && + !rtppayload_is_octet_aligned(resp_msg->data, resp_msg->len)) { + LOGP(DL1P, LOGL_NOTICE, + "%s RTP->L1: Dropping unexpected AMR encoding (bw-efficient?) %s\n", + gsm_lchan_name(lchan), osmo_hexdump(resp_msg->data, resp_msg->len)); + return false; + } + return true; +} + /* TCH-RTS-IND prim recevied from bts model */ static int l1sap_tch_rts_ind(struct gsm_bts_trx *trx, struct osmo_phsap_prim *l1sap, struct ph_tch_param *rts_ind) @@ -860,6 +901,10 @@ LOGP(DL1P, LOGL_DEBUG, "%s DL TCH Tx queue underrun\n", gsm_lchan_name(lchan)); resp_l1sap = &empty_l1sap; + } else if(!rtppayload_is_valid(lchan, resp_msg)) { + msgb_free(resp_msg); + resp_msg = NULL; + resp_l1sap = &empty_l1sap; } else { /* Obtain RTP header Marker bit from control buffer */ marker = rtpmsg_marker_bit(resp_msg); -- To view, visit https://gerrit.osmocom.org/6351 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 Gerrit-PatchSet: 2 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol
osmo-bts[master]: l1sap: Validate incoming RTP payload, drop bw-efficient AMR
Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/6351 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 Gerrit-PatchSet: 2 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol Gerrit-Reviewer: Harald Welte Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
osmo-bts[master]: l1sap: Validate incoming RTP payload, drop bw-efficient AMR
Patch Set 2: Code-Review+1 Verified+1 Tested, works fine. -- To view, visit https://gerrit.osmocom.org/6351 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 Gerrit-PatchSet: 2 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
[PATCH] osmo-bts[master]: l1sap: Validate incoming RTP payload, drop bw-efficient AMR
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/6351 to look at the new patch set (#2). l1sap: Validate incoming RTP payload, drop bw-efficient AMR A recurrent kernel crash in sysmobts (several kernel versions) corrupting kernel memory in random places has been investigated and reproduced by placing a call against an MSC sending RTP with bandwidth-efficient AMR payload to osmo-bts-sysmo. The osmo-bts-sysmo in turn sends the payload to the femtobts related kernel modules via a msgq, which most probably fail to handle correctly this bw-efficient AMR payload and corrupt the kernel memory. First approach was to drop the bw-efficient AMR payloads lower in the stack in sysmo specific code (l1if_tch_encode), but as there's no bts model in osmo-bts actually supporting bw-efficient AMR, let's drop it early in the incoming path for all models to avoid further problems. Related: SYS#4063 Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 --- M src/common/l1sap.c 1 file changed, 45 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/51/6351/2 diff --git a/src/common/l1sap.c b/src/common/l1sap.c index e181a73..1deee83 100644 --- a/src/common/l1sap.c +++ b/src/common/l1sap.c @@ -819,6 +819,47 @@ return 0; } +static bool rtppayload_is_octet_aligned(const uint8_t *rtp_pl, uint8_t payload_len) +{ + /* +* Logic: If 1st bit padding is not zero, packet is either: +* - bandwidth-efficient AMR payload. +* - malformed packet. +* However, Bandwidth-efficient AMR 4,75 frame last in payload(F=0, FT=0) +* with 4th,5ht,6th AMR payload to 0 matches padding==0. +* Furthermore, both AMR 4,75 bw-efficient and octet alignment are 14 bytes long (AMR 4,75 encodes 95b): +* bw-efficient: 95b, + 4b hdr + 6b ToC = 105b, + padding = 112b = 14B. +* octet-aligned: 1B hdr + 1B ToC + 95b = 111b, + padding = 112b = 14B. +* We cannot use other fields to match since they are inside the AMR +* payload bits which are unknown. +* As a result, this function may return false positive (true) for some AMR +* 4,75 AMR frames, but given the length, CMR and FT read is the same as a +* consequence, the damage in here is harmless other than being unable to +* decode the audio at the other side. +*/ + #define AMR_PADDING1(rtp_pl) (rtp_pl[0] & 0x0f) + #define AMR_PADDING2(rtp_pl) (rtp_pl[1] & 0x03) + + if(payload_len < 2 || AMR_PADDING1(rtp_pl) || AMR_PADDING2(rtp_pl)) + return false; + + return true; +} + +static bool rtppayload_is_valid(struct gsm_lchan *lchan, struct msgb *resp_msg) +{ + /* Avoid sending bw-efficient AMR to lower layers, most bts models +* don't support it. */ + if(lchan->tch_mode == GSM48_CMODE_SPEECH_AMR && + !rtppayload_is_octet_aligned(resp_msg->data, resp_msg->len)) { + LOGP(DL1P, LOGL_NOTICE, + "%s RTP->L1: Dropping unexpected AMR encoding (bw-efficient?) %s\n", + gsm_lchan_name(lchan), osmo_hexdump(resp_msg->data, resp_msg->len)); + return false; + } + return true; +} + /* TCH-RTS-IND prim recevied from bts model */ static int l1sap_tch_rts_ind(struct gsm_bts_trx *trx, struct osmo_phsap_prim *l1sap, struct ph_tch_param *rts_ind) @@ -860,6 +901,10 @@ LOGP(DL1P, LOGL_DEBUG, "%s DL TCH Tx queue underrun\n", gsm_lchan_name(lchan)); resp_l1sap = &empty_l1sap; + } else if(!rtppayload_is_valid(lchan, resp_msg)) { + msgb_free(resp_msg); + resp_msg = NULL; + resp_l1sap = &empty_l1sap; } else { /* Obtain RTP header Marker bit from control buffer */ marker = rtpmsg_marker_bit(resp_msg); -- To view, visit https://gerrit.osmocom.org/6351 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 Gerrit-PatchSet: 2 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol
osmo-bts[master]: l1sap: Validate incoming RTP payload, drop bw-efficient AMR
Patch Set 1: Code-Review-1 -1 As I could test it only with a non-bw-efficient MSC but I'm having problems right now connecting to the offending MSC sending bw-efficient mode. Once I test it there (I expect no major problems since I already tested a previous version of the patch), we can then merge it. -- To view, visit https://gerrit.osmocom.org/6351 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 Gerrit-PatchSet: 1 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Pau Espin Pedrol Gerrit-HasComments: No
[PATCH] osmo-bts[master]: l1sap: Validate incoming RTP payload, drop bw-efficient AMR
Review at https://gerrit.osmocom.org/6351 l1sap: Validate incoming RTP payload, drop bw-efficient AMR A recurrent kernel crash in sysmobts (several kernel versions) corrupting kernel memory in random places has been investigated and reproduced by placing a call against an MSC sending RTP with bandwidth-efficient AMR payload to osmo-bts-sysmo. The osmo-bts-sysmo in turn sends the payload to the femtobts related kernel modules via a msgq, which most probably fail to handle correctly this bw-efficient AMR payload and corrupt the kernel memory. First approach was to drop the bw-efficient AMR payloads lower in the stack in sysmo specific code (l1if_tch_encode), but as there's no bts model in osmo-bts actually supporting bw-efficient AMR, let's drop it early in the incoming path for all models to avoid further problems. Related: SYS#4063 Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 --- M src/common/l1sap.c 1 file changed, 43 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/51/6351/1 diff --git a/src/common/l1sap.c b/src/common/l1sap.c index e181a73..3ff7105 100644 --- a/src/common/l1sap.c +++ b/src/common/l1sap.c @@ -819,6 +819,47 @@ return 0; } +static bool rtppayload_is_octet_aligned(const uint8_t *rtp_pl, uint8_t payload_len) +{ + /* +* Logic: If 1st bit padding is not zero, packet is either: +* - bandwidth-efficient AMR payload. +* - malformed packet. +* However, Bandwidth-efficient AMR 4,75 frame last in payload(F=0, FT=0) +* with 4th,5ht,6th AMR payload to 0 matches padding==0. +* Furthermore, both AMR 4,75 bw-efficient and octet alignment are 14 bytes long (AMR 4,75 encodes 95b): +* bw-efficient: 95b, + 4b hdr + 6b ToC = 105b, + padding = 112b = 14B. +* octet-aligned: 1B hdr + 1B ToC + 95b = 111b, + padding = 112b = 14B. +* We cannot use other fields to match since they are inside the AMR +* payload bits which are unknown. +* As a result, this function may return false positive (true) for some AMR +* 4,75 AMR frames, but given the length, CMR and FT read is the same as a +* consequence, the damage in here is harmless other than being unable to +* decode the audio at the other side. +*/ + #define AMR_PADDING1(rtp_pl) (rtp_pl[0] & 0x0f) + #define AMR_PADDING2(rtp_pl) (rtp_pl[1] & 0x03) + + if(payload_len < 2 || AMR_PADDING1(rtp_pl) || AMR_PADDING2(rtp_pl)) + return false; + + return true; +} + +static bool rtppayload_is_valid(struct gsm_lchan *lchan, struct msgb *resp_msg) +{ + /* Avoid sending bw-efficient AMR to lower layers, most bts models +* don't support it. */ + if(lchan->tch_mode == GSM48_CMODE_SPEECH_AMR && + !rtppayload_is_octet_aligned(resp_msg->data, resp_msg->len)) { + LOGP(DL1P, LOGL_NOTICE, + "%s RTP->L1: Dropping unexpected AMR encoding (bw-efficient?) %s\n", + gsm_lchan_name(lchan), osmo_hexdump(resp_msg->data, resp_msg->len)); + return false; + } + return true; +} + /* TCH-RTS-IND prim recevied from bts model */ static int l1sap_tch_rts_ind(struct gsm_bts_trx *trx, struct osmo_phsap_prim *l1sap, struct ph_tch_param *rts_ind) @@ -860,6 +901,8 @@ LOGP(DL1P, LOGL_DEBUG, "%s DL TCH Tx queue underrun\n", gsm_lchan_name(lchan)); resp_l1sap = &empty_l1sap; + } else if(!rtppayload_is_valid(lchan, resp_msg)) { + resp_l1sap = &empty_l1sap; } else { /* Obtain RTP header Marker bit from control buffer */ marker = rtpmsg_marker_bit(resp_msg); -- To view, visit https://gerrit.osmocom.org/6351 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If0c9233c628c724de4ab74e58e3e2affac79e6d0 Gerrit-PatchSet: 1 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol