[Bug report] nRF SDK GPIOTE bug

[trentrolf]

Bug in the SDK function below. channel_free is called whether or not pin_in_use_by_te() is true or not. For us this resulted in channel_free() getting called with an invalid channel_port of FF which resulted in an out of bounds dereference and memory corruption. I would suggest only calling channel_free() if pin_in_use_by_te() is true.

We found this in v13.0.0, but it looks like it's still in the latest version.

I'm aware that we probably shouldn't be calling uninit on a function that hasn't had init called first, but regardless I think the protection would be wise.

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);
}

[Stian Røed Hafskjold]

Short update on this: This was never fixed because this is not considered to be a bug - if `channel_port_get` returns 0xFF it means that user attempts to un-initialize a pin that was not initialized before. This is not a correct use-case and it is guarded by the assertion: https://github.com/NordicSemiconductor/nrfx/blob/d779b49fc59c7a165e7da1d7cd7d57b28a059f16/drivers/src/nrfx_gpiote.c#L805

[trentrolf]

I am aware that I shouldn't have called uninit before init. I mentioned that in the initial report. However you cannot rely on ASSERT for the bounds check. ASSERTs are not compiled into release builds. The unique circumstances for this bug to occur did not surface during our debug development and testing, and resulted in a disastrous out-of-bounds memory modification. 

You are already performing the bounds check below the assert and it would cost nothing to move the free within that conditional.

[Stian Røed Hafskjold]

It will have to be checked by pin_in_use_by_gpiote(pin) not pin_in_use_by_te(pin) since the pin can be used with the port event as well. The alternative is to change the ASSERT to a conditional, but then the function would silently fail (return value is void), which is not ideal either.

[trentrolf]

Yes I agree it is not optimal, but better than an out-of-bounds memory reference. Actually it is not a "fail", it's just "no action" which for me would be the expected behavior when you call uninit() on any module that has not been initialized.

[Stian Røed Hafskjold]

Yes, I agree. I have forwarded your input to the responsible team.