Failure in nrfx_gpiote

[Andrej]

Hello,

in file nrfx_gpiote.c there is a fatal error which overrides data because of a false array index.

• nrfx_gpiote_init there is a for loop which calls the function channel_free with i from 0 to GPIOTE_CH_NUM +  NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS , but in channel_free the array m_cb.port_handlers_pins is only of size NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS

•  in another case calling multiple times nrfx_gpiote_in_uninit-->  channel_free gets channel_id 0xFF  which is greater than GPIOTE_CH_NUM which results in a wrong array index, which overrides some data in ram

I looked in SDK Version 15.2.0 and 17.0.2 both have these issues and even more.

Could you pls fix these in future SDKs?

Best regards 

Andrej

some code snippets out of nrfx_gpiote

typedef struct
{
    nrfx_gpiote_evt_handler_t handlers[GPIOTE_CH_NUM + NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS];
    int8_t                    pin_assignments[MAX_PIN_NUMBER];
    int8_t                    port_handlers_pins[NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS];
    uint8_t                   configured_pins[((MAX_PIN_NUMBER)+7) / 8];
    nrfx_drv_state_t          state;
} gpiote_control_block_t;

static gpiote_control_block_t m_cb;

nrfx_err_t nrfx_gpiote_init(void)
{
    nrfx_err_t err_code;

    if (m_cb.state != NRFX_DRV_STATE_UNINITIALIZED)
    {
        err_code = NRFX_ERROR_INVALID_STATE;
        NRFX_LOG_WARNING("Function: %s, error code: %s.",
                         __func__,
                         NRFX_LOG_ERROR_STRING_GET(err_code));
        return err_code;
    }

    uint8_t i;

    for (i = 0; i < MAX_PIN_NUMBER; i++)
    {
        if (nrf_gpio_pin_present_check(i))
        {
            pin_in_use_clear(i);
        }
    }

    for (i = 0; i < (GPIOTE_CH_NUM + NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS); i++)
    {
        channel_free(i);
    }

    memset(m_cb.configured_pins, 0, sizeof(m_cb.configured_pins));

    NRFX_IRQ_PRIORITY_SET(nrfx_get_irq_number(NRF_GPIOTE), NRFX_GPIOTE_CONFIG_IRQ_PRIORITY);
    NRFX_IRQ_ENABLE(nrfx_get_irq_number(NRF_GPIOTE));
    nrf_gpiote_event_clear(NRF_GPIOTE_EVENTS_PORT);
    nrf_gpiote_int_enable(GPIOTE_INTENSET_PORT_Msk);
    m_cb.state = NRFX_DRV_STATE_INITIALIZED;

    err_code = NRFX_SUCCESS;
    NRFX_LOG_INFO("Function: %s, error code: %s.", __func__, NRFX_LOG_ERROR_STRING_GET(err_code));
    return err_code;
}

void nrfx_gpiote_in_uninit(nrfx_gpiote_pin_t pin)
{
    NRFX_ASSERT(nrf_gpio_pin_present_check(pin));
    NRFX_ASSERT(pin_in_use_by_gpiote(pin));
    nrfx_gpiote_in_event_disable(pin);
    if (pin_in_use_by_te(pin))
    {
        nrf_gpiote_te_default((uint32_t)channel_port_get(pin));
    }
    if (pin_configured_check(pin))
    {
        nrf_gpio_cfg_default(pin);
        pin_configured_clear(pin);
    }
    channel_free((uint8_t)channel_port_get(pin));
    pin_in_use_clear(pin);
}

__STATIC_INLINE int8_t channel_port_get(uint32_t pin)
{
    return m_cb.pin_assignments[pin];
}

static void channel_free(uint8_t channel_id)
{
    m_cb.handlers[channel_id] = FORBIDDEN_HANDLER_ADDRESS;
    if (channel_id >= GPIOTE_CH_NUM)
    {
        m_cb.port_handlers_pins[channel_id - GPIOTE_CH_NUM] = (int8_t)PIN_NOT_USED;
    }
}

[Jared]

Hi,

Reply from our SW engineers in bold:

Andrej said:

nrfx_gpiote_init there is a for loop which calls the function channel_free with i from 0 to GPIOTE_CH_NUM +  NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS , but in channel_free the array m_cb.port_handlers_pins is only of size NRFX_GPIOTE_CONFIG_NUM_OF_LOW_POWER_EVENTS

static void channel_free(uint8_t channel_id)
{
    m_cb.handlers[channel_id] = FORBIDDEN_HANDLER_ADDRESS;
    if (channel_id >= GPIOTE_CH_NUM)
    {
        m_cb.port_handlers_pins[channel_id - GPIOTE_CH_NUM] = (int8_t)PIN_NOT_USED;
    }
}

• m_cb.port_handlers_pins is accessed only for channel number higher than GPIOTE_CH_NUM, and with index channel_id - GPIOTE_CH_NUM so no overflow occurs

Andrej said:

 in another case calling multiple times nrfx_gpiote_in_uninit-->  channel_free gets channel_id 0xFF  which is greater than GPIOTE_CH_NUM which results in a wrong array index, which overrides some data in ram

• this is true, but user is not allowed to uninitialize already uninitialized pin and this is checked with an assertion:

void nrfx_gpiote_in_uninit(nrfx_gpiote_pin_t pin)
{
    (...)
    NRFX_ASSERT(pin_in_use_by_gpiote(pin));

Have you actually reproduced an issue that is caused by any of two points that you mention?

Andrej said:

I looked in SDK Version 15.2.0 and 17.0.2 both have these issues and even more.

What do you mean by "even more", could you elaborate on this? 

[Andrej]

Okay the first issue was my fault and I am sorry for that.

My workmate was confused, because it ran into channel_free with 0xFF even with DEBUG_NRF enabled.

He uses an example project from the sdk with some additional code.

Without DEBUG_NRF or DEBUG_NRF_USER there was no error message.

We thought that there is some kind of smart code which detect's already uninit pins and skips execution of uninit.

As i see all information are already in this modul to check, so why the user must remember it and wasting resources?

The function pin_in_use_by_gpiote is static in nrfx_gpiote.c so there is no way to check this before calling the uninit function.

If i am wrong, please help me to understand.

With "even more" i just wanted to say that i don't checked the whole c file

[Jared]

I don't think that it's unreasonable to expect the developer to not call the uninit function on already un initialized pins, or do a simple check with pin_in_use_by_gpiote() before calling it. If the former is done, then the application will assert with NRFX_ASSERT(pin_in_use_by_gpiote(pin))

[Andrej]

As I said the function pin_in_use_by_gpiote() can't be called because it's not declared in header-file,

so we should work on our code. thanks for helping.