osmo_fsm timer_cb fudge up

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.

Neels Hofmeyr nhofmeyr at sysmocom.de
Sat Nov 18 22:16:50 UTC 2017


I'm finding heap use after free related to osmo_fsm:

- When dispatching a timer_cb event, we want to set fi->T back to 0. (see fsm_tmr_cb())

- So that we know which timer fired, we leave fi->T at its current value for
  the timer_cb to be able to make decisions based on that. This is used for
  example in fsm_timeout_cb() in osmo-bsc/osmo_bsc_mgcp.c.

- In other code, the timer_cb event may decide to fire events or call functions
  which effect an FSM instance teardown and deallocation, meaning that in
  fsm_tmr_cb(), we are *not* allowed to generally assume the fi still exists.
  If we set fi->T = 0 after the timer_cb() was called, we may write to freed
  mem.

- Also, if the timer_cb effects events with their own timeout, we will also
  overwrite the fi->T value with zero after another timer has set its value in
  T, causing failures to evaluate that timer later on.

Solutions so far:

- set fi->T = 0 first, and pass T to the timer_cb as arg instead. I think this
  is the best and cleanest, but also the hardest in terms of backwards compat.
  I have a patch that adds timer_cb2() that features a T arg; we need to submit
  that, move all code that wants to use T to that timer_cb2(), and then we can
  move the fi->T = 0 to before the timer_cb(). Actually, this is not being
  backwards compatible at all, because all code trying to use fi->T as usual
  will need adjustment, we break API either way and might as well add T to the
  timer_cb directly instead... :/

- Actually not set fi->T = 0 at all. We will have T holding the value of the
  timer that fired last, all old code will still find this T in fi->T, and
  after the cb, we just leave it as it was. Does it really need to be cleared?
  This has no compatibility issues and solves the use after free as well as
  setting a new T from timer_cb() issues. Going for this one so far.

Thoughts?

~N
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.osmocom.org/pipermail/openbsc/attachments/20171118/0bc20530/attachment.bin>


More information about the OpenBSC mailing list