nrf5x: fix array overflow in gpio configuration code

[andrewleech]

Description

nrf5x: gpiote uninit only needs to be run if input pin is configured for irq (or output)

Refer to #6020 for more details

Status

READY

Migrations

NO

Related PRs

#5207

[mbed-ci]

User not whitelisted, CI not run.

[0xc0170]

@ARMmbed/team-nordic Please review

[marcuschangarm]

@andrewleech

What target are you running this on? Do you have a code snippet that triggers this bug? Thanks!

[andrewleech]

I'm working with a nrf51 using MCU_NRF51_UNIFIED but anything using TARGET_NRF5 should be affected since #5207
I've only confirmed this on TARGET_SDK11, not sure if any of the lower implementation changed on SDK13 to protect against this.

It happens the very first time a DigitalIn resource is defined in the project (and every time after that).

I've just replicated it with a new project on nrf51_dk with the following main.cpp:

#include <mbed.h>
DigitalIn pin(BUTTON1);
int main(void) {
    while (1) ;
}

mbed-os updated to master
pushd mbed-os; git fetch; git checkout master; popd
With a debug build
mbed compile --profile mbed-os/tools/profiles/debug.json -m NRF51_DK

You'll need to be debugging to see the problem, if you apply this patch in mbed-os folder it'll break at the crucial point:

diff --git a/targets/TARGET_NORDIC/TARGET_NRF5/TARGET_SDK11/drivers_nrf/gpiote/nrf_drv_gpiote.c b/targets/TARGET_NORDIC/TARGET_NRF5/TARGET_SDK11/drivers_nrf/gpiote/nrf_drv_gpiote.c
index 752c89c9a..417962c68 100644
--- a/targets/TARGET_NORDIC/TARGET_NRF5/TARGET_SDK11/drivers_nrf/gpiote/nrf_drv_gpiote.c
+++ b/targets/TARGET_NORDIC/TARGET_NRF5/TARGET_SDK11/drivers_nrf/gpiote/nrf_drv_gpiote.c
@@ -465,6 +465,7 @@ void nrf_drv_gpiote_in_uninit(nrf_drv_gpiote_pin_t pin)
         nrf_gpiote_te_default(channel_port_get(pin));
     }
     nrf_gpio_cfg_default(pin);
+    __asm("BKPT #0\n");
     channel_free((uint8_t)channel_port_get(pin));
     pin_in_use_clear(pin);
 }

If you look in mbed-os\hal\mbed_gpio.c -> _gpio_init_in() you'll see it runs gpio_dir() then gpio_mode().

gpio_dir() marks the pin as configured, so when gpio_mode() runs it tries to de-init it before reconfiguring. The deinit calls nrf_drv_gpiote_in_uninit() however as the gpiote has not been used for this digitalin, when the line channel_free((uint8_t)channel_port_get(pin)); is run, channel_port_get(pin) returns -1 so channel_free() writes 0xffffffff to a handlers array at index -1 (255) (however it's only 12 long).

Where this out of bounds index is actually pointing is program specific so may not be immediately obvious that something's gone wrong, but your runtime is now corrupted.

If you enable nordic asserts (aka #6022) there is a check at the top of nrf_drv_gpiote_in_uninit() that protects against this exact issue: ASSERT(pin_in_use_by_gpiote(pin));

[marcuschangarm]

It looks good to me, but to be honest my experience with the gpio_api.c implementation is quite limited.

Did you at some point consider adding a catch-all else statement?

        if ((m_gpio_cfg[pin].direction == PIN_OUTPUT) && (!m_gpio_cfg[pin].used_as_irq)) {
            nrf_drv_gpiote_out_uninit(pin);
        }
        else if (m_gpio_cfg[pin].used_as_irq) {
            nrf_drv_gpiote_in_uninit(pin);
        else {
            nrf_gpio_cfg_default(pin);
        }

[andrewleech]

I don't think we want to reset the pin to defaults at all, this function is simply to detach any used gpiote's before they're reassigned later (if needed)

[marcuschangarm]

I was just looking at the functions inside nrf_drv_gpiote_in_uninit, and saw that nrf_gpio_cfg_default was being called as well.

How did this work in the first place then? 😄

[andrewleech]

Ah yes, I see what you mean.
nrf_gpiote_te_default() is only called (in nrf_drv_gpiote_in_uninit()) when the pin is actually configured for use by the gpiote module (as opposed to regular gpio module).
fyi: gpiote is for advanced pin functions like interrupts / autoamated tasks.

Prior to #5207 a gpiote channel was used for every gpio allocation (DigitalIn, DigitalOut, InterruptIn, InterruptOut). This meant that it was always safe to unallocate the gpiote because there was always one allocated.

gpiote channels are a limited resource however, hence #5207 (don't use a gpiote channel when not needed for a basic digitalin gpio).

[marcuschangarm]

I see! Thank you for the explanation!

[0xc0170]

/morph build

[mbed-ci]

Build : SUCCESS

Build number : 1213
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/6021/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

[mbed-ci]

Exporter Build : SUCCESS

Build number : 884
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/6021/

[mbed-ci]

Test : FAILURE

Build number : 1014
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6021/1014

[0xc0170]

/morph test

[mbed-ci]

Test : SUCCESS

Build number : 1022
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6021/1022