voicecall: Fix use after free

Commit Message

Denis Kenzior May 28, 2024, 10:06 p.m. UTC

On shutdown, if the sim atom is removed before the voicecall atom, the
sim context is removed automatically.  However, voicecall atom does set
sim_context to NULL in that case, resulting in the following valgrind
report:

==251105== Invalid read of size 8
==251105==    at 0x59B4F3: sim_fs_file_watch_remove (src/simfs.c:243)
==251105==    by 0x560E2A: ofono_sim_remove_file_watch (src/sim.c:2621)
==251105==    by 0x5412E3: unwatch_sim_ecc_numbers (src/voicecall.c:2820)
==251105==    by 0x53B8C8: voicecall_unregister (src/voicecall.c:2849)
==251105==    by 0x52B16B: __ofono_atom_unregister (src/modem.c:336)
==251105==    by 0x52E6E4: flush_atoms (src/modem.c:492)
==251105==    by 0x52C1BE: modem_change_state (src/modem.c:586)
==251105==    by 0x52E155: set_powered (src/modem.c:974)
==251105==    by 0x52E307: __ofono_modem_shutdown (src/modem.c:2279)
==251105==    by 0x5296B6: signal_handler (src/main.c:85)
==251105==    by 0x48FF198: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
==251105==    by 0x495E3BE: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
==251105==  Address 0x5baa6f8 is 8 bytes inside a block of size 16 free'd
==251105==    at 0x48458CF: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==251105==    by 0x59B2A1: sim_fs_context_free (src/simfs.c:213)
==251105==    by 0x59B133: sim_fs_free (src/simfs.c:123)
==251105==    by 0x5620EF: sim_remove (src/sim.c:3239)
==251105==    by 0x52E701: flush_atoms (src/modem.c:495)
==251105==    by 0x52C1BE: modem_change_state (src/modem.c:586)
==251105==    by 0x52E155: set_powered (src/modem.c:974)
==251105==    by 0x52E307: __ofono_modem_shutdown (src/modem.c:2279)
==251105==    by 0x5296B6: signal_handler (src/main.c:85)
==251105==    by 0x48FF198: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
==251105==    by 0x495E3BE: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
==251105==    by 0x48FFDC6: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.8000.0)
---
 src/voicecall.c | 1 +
 1 file changed, 1 insertion(+)

Comments

[e-mail] May 30, 2024, 2:40 p.m. UTC | #1

Hello:

This patch was applied to ofono.git (master)
by Denis Kenzior <[e-mail]>:

On Tue, 28 May 2024 17:06:29 -0500 you wrote:
> On shutdown, if the sim atom is removed before the voicecall atom, the
> sim context is removed automatically.  However, voicecall atom does set
> sim_context to NULL in that case, resulting in the following valgrind
> report:
> 
> ==251105== Invalid read of size 8
> ==251105==    at 0x59B4F3: sim_fs_file_watch_remove (src/simfs.c:243)
> ==251105==    by 0x560E2A: ofono_sim_remove_file_watch (src/sim.c:2621)
> ==251105==    by 0x5412E3: unwatch_sim_ecc_numbers (src/voicecall.c:2820)
> ==251105==    by 0x53B8C8: voicecall_unregister (src/voicecall.c:2849)
> ==251105==    by 0x52B16B: __ofono_atom_unregister (src/modem.c:336)
> ==251105==    by 0x52E6E4: flush_atoms (src/modem.c:492)
> ==251105==    by 0x52C1BE: modem_change_state (src/modem.c:586)
> ==251105==    by 0x52E155: set_powered (src/modem.c:974)
> ==251105==    by 0x52E307: __ofono_modem_shutdown (src/modem.c:2279)
> ==251105==    by 0x5296B6: signal_handler (src/main.c:85)
> ==251105==    by 0x48FF198: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
> ==251105==    by 0x495E3BE: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
> ==251105==  Address 0x5baa6f8 is 8 bytes inside a block of size 16 free'd
> ==251105==    at 0x48458CF: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==251105==    by 0x59B2A1: sim_fs_context_free (src/simfs.c:213)
> ==251105==    by 0x59B133: sim_fs_free (src/simfs.c:123)
> ==251105==    by 0x5620EF: sim_remove (src/sim.c:3239)
> ==251105==    by 0x52E701: flush_atoms (src/modem.c:495)
> ==251105==    by 0x52C1BE: modem_change_state (src/modem.c:586)
> ==251105==    by 0x52E155: set_powered (src/modem.c:974)
> ==251105==    by 0x52E307: __ofono_modem_shutdown (src/modem.c:2279)
> ==251105==    by 0x5296B6: signal_handler (src/main.c:85)
> ==251105==    by 0x48FF198: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
> ==251105==    by 0x495E3BE: ??? (in /usr/lib/libglib-2.0.so.0.8000.0)
> ==251105==    by 0x48FFDC6: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.8000.0)
> 
> [...]

Here is the summary with links:
  - voicecall: Fix use after free
    https://git.kernel.org/pub/scm/network/ofono/ofono.git/?id=da1e4440a17b

You are awesome, thank you!

Patch

diff --git a/src/voicecall.c b/src/voicecall.c
index d9f3dd82f0e6..398a576a3cc2 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -2966,6 +2966,7 @@  static void sim_watch(struct ofono_atom *atom,
 
 	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) {
 		voicecall_close_settings(vc);
+		vc->sim_context = NULL;
 		vc->sim_state_watch = 0;
 		vc->sim = NULL;
 		return;