RFC fix last coverity report with SI handling in osmo-bts
Hi, the coverity tools points out that it might be possible to access the si.buf atfer _MAX_SYSINFO_TYPE and before < 32. Looking at the code I also notice that: osmo_rsl2sitype takes an uint8_t but makes no range checking, same goes for osmo_sitype2rsl. I plan to apply the below patch for osmo-bts. This would resolve the last open Coverity report for osmo-bts. It would be nice if we could add range checking to osmo_rsl2sitype or check it in common/rsl.c of osmo-bts. diff --git a/src/common/sysinfo.c b/src/common/sysinfo.c index 62d2af9..9e2d4bb 100644 --- a/src/common/sysinfo.c +++ b/src/common/sysinfo.c @@ -135,7 +135,7 @@ uint8_t *lchan_sacch_get(struct gsm_lchan *lchan, struct gsm_time *g_time) { uint32_t tmp; - for (tmp = lchan->si.last + 1; tmp != lchan->si.last; tmp = (tmp + 1) % 32) { + for (tmp = lchan->si.last + 1; tmp != lchan->si.last; tmp = (tmp + 1) % _MAX_SYSINFO_TYPE) { if (lchan->si.valid & (1 << tmp)) { lchan->si.last = tmp; return lchan->si.buf[tmp];
Hi Holger, On Thu, Jan 02, 2014 at 08:35:54PM +0100, Holger Hans Peter Freyther wrote:
osmo_rsl2sitype takes an uint8_t but makes no range checking, same goes for osmo_sitype2rsl.
I plan to apply the below patch for osmo-bts.
I see no problem with that.
This would resolve the last open Coverity report for osmo-bts. It would be nice if we could add range checking to osmo_rsl2sitype or check it in common/rsl.c of osmo-bts.
The osmo_rsl2sitype(uint8_5 rsl_si) function is using the rsl_si as index into the rsl2sitype[256] array, so it cannot cause any array out of bounds access, from what I can tell. If we add range checking, then we would also have to specify an error return code that would express the error from having occurred. So far, SYSINFO_TYPE_NONE is defined as zero, i.e. will be returned for every value between _MAX_SYSINFO_TYPE and 265. As for osmo_sitype2rsl: Yes, indeed, if you pass a value outside the enum osmo_sysinfo_type into the function, we may index sitype2rsl[] out of bounds. The question is what to return? RSL of course has no definition for a RSL_SYSTEM_INFO_INVALID or the like. So unless we change the function prototype to return an integer type, we cannot return -1... Regards, Harald -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
participants (2)
-
Harald Welte -
Holger Hans Peter Freyther