jakkra / ZSWatch

ZSWatch - the Open Source Zephyr™ based Smartwatch, including both HW and FW.
https://forms.gle/G48Sm5zDe9aCaYtT9
GNU General Public License v3.0
2.33k stars 202 forks source link

Possible Bug in z_nrf_clock_control_lf_on #360

Open PwnVerse opened 3 hours ago

PwnVerse commented 3 hours ago

In the following function -

void z_nrf_clock_control_lf_on(enum nrf_lfclk_start_mode start_mode)
{
    static atomic_t on;
    static struct onoff_client cli;

    if (atomic_set(&on, 1) == 0) {
        int err;
        struct onoff_manager *mgr =
                get_onoff_manager(CLOCK_DEVICE,
                          CLOCK_CONTROL_NRF_TYPE_LFCLK);

        sys_notify_init_spinwait(&cli.notify);
        err = onoff_request(mgr, &cli);
        __ASSERT_NO_MSG(err >= 0);
    }
}

A pointer to mgr is acquired from get_onoff_manager and then passed on to onoff_request which eventually calls process_event with the same mgr as one it's arguments.

int onoff_request(struct onoff_manager *mgr,
          struct onoff_client *cli)
{
    bool start = false;             /* trigger a start transition */

    uint32_t state = mgr->flags & ONOFF_STATE_MASK;
       // Other checks before and after this
    if ((state == ONOFF_STATE_OFF)
           || (state == ONOFF_STATE_TO_OFF)
           || (state == ONOFF_STATE_TO_ON)) {
        /* Start if OFF, queue client */
        start = (state == ONOFF_STATE_OFF);
        add_client = true;

out:
    if (add_client) {
        sys_slist_append(&mgr->clients, &cli->node);
    }

    if (start) {
        process_event(mgr, EVT_RECHECK, key);
}

If mgr is uninitialized at this point, that means all of it's members are NULL and this can cause crashes when accessing mgr->transitions->start.

static void process_event(struct onoff_manager *mgr,
              int evt,
              k_spinlock_key_t key)
{
      ... not relevant code...
    do {
        .. some other operations...

            transit = mgr->transitions->start; // crash if mgr->transitions is uninitialized
}

Maybe I'm getting the flow wrong, but if considered from entrypoint c_zstart after the reset vectors is called, a call to z_sys_init_run_level eventually calls sys_clock_driver_init that calls the function of interest. In this entire flow (atleast statically), I dont see transitions being initialized.

jakkra commented 2 hours ago

Hi, not sure why you create this issue in my repo, it should go to the Zephyr repo instead as this is part of that codebase. Are you getting a crash in ZSWatch codebase and have debugged it to this?