Attention is currently required from: jolly, neels, laforge, pespin, fixeria.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31417 ) Change subject: l1sap: Drop unsupported HR GSM payload ...................................................................... Patch Set 9: (7 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/967db791_44a14ff8 PS8, Line 16: are accepted. > Agree, I also mentioned that in my last review. In l1sap_rtp_rx_cb() we convert the payload but the conversion is only carried out when there is an exact length match. If there are packets with invalid payloads (either too long or to short) received they would slip through. This code path is not about gatekeeping, its only about the conversion. in rtppayload_is_valid() we make sure that the payload length matches the expectations of the BTS and ensures that no unsupported payload is passed on to lower layers. That are essentially two different topics and I think it makes sense to split those in separate patches. I have spitted the conversion part in a separate patch. File src/common/bts.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/069c0992_729468a0 PS8, Line 207: " > add "...is supported" or "support for ... […] Done https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/94c8df84_7cbb2095 PS8, Line 208: t > same Done File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/1f78d62d_2270d00b PS7, Line 1274: if (OSMO_UNLIKELY(rfc5993 && (resp_msg->len != GSM_HR_BYTES_RTP_RFC5993))) { > I agree that a flag being zero should disallow that format, no matter what > the other flag's value is […] I think this is fixed now. In case no format is set both payload formats would be rejected. So a the hardware specific code now must have the flags set accordingly. File src/common/l1sap.c: https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/f6e84a99_ba4af586 PS8, Line 1274: /* BTS flags specify that RFC 5993 (and not ETSI TS 101.318) is understood by lower layers */ > Ack Done https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/e8e9663d_c97bc7c9 PS8, Line 1952: * formats are supported (either by setting both or none of the flags), no conversion will be carried out. */ > Ack I have splatted the converting part into a follow up patch - so it is rejecting https://gerrit.osmocom.org/c/osmo-bts/+/31417/comment/4e9352c1_ea24eff6 PS8, Line 1969: break; > the code converting from one format to the other should be in separate > functions This turns out even more ugly due to the l1sap_msgb_alloc and the fallthrough. Anyway, lets discuss this in: I9419b40c1171876879d41aba4f51c93e8ef5673c then -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31417 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I453562da412fde5b928bd2b588129c58ec8e2a7e Gerrit-Change-Number: 31417 Gerrit-PatchSet: 9 Gerrit-Owner: dexter <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: jolly <andr...@eversberg.eu> Gerrit-Reviewer: laforge <lafo...@osmocom.org> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: fixeria <vyanits...@sysmocom.de> Gerrit-CC: neels <nhofm...@sysmocom.de> Gerrit-Attention: jolly <andr...@eversberg.eu> Gerrit-Attention: neels <nhofm...@sysmocom.de> Gerrit-Attention: laforge <lafo...@osmocom.org> Gerrit-Attention: pespin <pes...@sysmocom.de> Gerrit-Attention: fixeria <vyanits...@sysmocom.de> Gerrit-Comment-Date: Fri, 05 May 2023 10:00:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofm...@sysmocom.de> Comment-In-Reply-To: pespin <pes...@sysmocom.de> Comment-In-Reply-To: dexter <pma...@sysmocom.de> Gerrit-MessageType: comment
