Possible Null dereference in z_nrf_clock_control_lf_on

[PwnVerse]

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.

[github-actions[bot]]

Hi @PwnVerse! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[nordicjm]

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.

Why would it be uninitialised?

static struct onoff_manager *get_onoff_manager(const struct device *dev,
                                               enum clock_control_nrf_type type)
{
        struct nrf_clock_control_data *data = dev->data;

        return &data->mgr[type];
}

[anangl]

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.

sys_clock_driver_init() is called in PRE_KERNEL_2, so after the clock_control_nrf driver is initialized in PRE_KERNEL_1, thus after: https://github.com/zephyrproject-rtos/zephyr/blob/2bfc2a3cc57f37a29d988092ce3c127ce9de2274/drivers/clock_control/clock_control_nrf.c#L678-L684

[PwnVerse]

For the application (ZSWatch) that i compiled with default configurations, both __init_EARLY_start and __init_PRE_KERNEL_1_start point to __init_pthread_barrier_pool_init -

gef➤  p &__init_EARLY_start
$1 = (const struct init_entry (*)[]) 0x88a00 <__init_pthread_barrier_pool_init>
gef➤   p &__init_PRE_KERNEL_1_start
$2 = (const struct init_entry (*)[]) 0x88a00 <__init_pthread_barrier_pool_init>
gef➤  p &__init_PRE_KERNEL_2_start
$3 = (const struct init_entry (*)[]) 0x88a90 <__init_sys_clock_driver_init>
gef➤   p &__init_POST_KERNEL_start
$4 = (const struct init_entry (*)[]) 0x88a98 <__init_rtc_pretick_init>
gef➤   p &__init_APPLICATION_start
$5 = (const struct init_entry (*)[]) 0x88c20 <__init_zsw_timer_init>

This is further re-instated by loading the ARM ELF in IDA-pro and looking at the levels array -

rodata:000C2E3C levels.0        DCD __init_pthread_barrier_pool_init
rodata:000C2E3C                                         ; DATA XREF: z_sys_init_run_level+2↑o
rodata:000C2E3C                                         ; text:off_7DBE0↑o
rodata:000C2E40                 DCD __init_pthread_barrier_pool_init
rodata:000C2E44                 DCD __init_sys_clock_driver_init
rodata:000C2E48                 DCD __init_rtc_pretick_init
rodata:000C2E4C                 DCD __init_zsw_timer_init
rodata:000C2E50                 DCD __deferred_init_list_end

In this case, for some reason __init_PRE_KERNEL_1_start is not pointing to clk_init as defined in clock_control_nrf.c -

DEVICE_DT_DEFINE(DT_NODELABEL(clock), clk_init, NULL,
         &data, &config,
         PRE_KERNEL_1, CONFIG_CLOCK_CONTROL_INIT_PRIORITY,
         &clock_control_api);

Here is how I have compiled the application -

git clone https://github.com/jakkra/ZSWatch.git
cd ZSWatch
git submodule update --init --recursive
cd app
west init -l .
west update
cd ..
west build app -p -b zswatch_nrf5340_cpuapp@5 -- -DOVERLAY_CONFIG=boards/release.conf -DBOARD_ROOT=${PWD}/app