Bug in how bootloader treats GPREGRET

[Jonathan]

I suspect this is related to https://devzone.nordicsemi.com/f/nordic-q-a/36601/gpregret-and-nrf_bootloader-c-evolution-proposal (Case ID: 211047)

In nrf_bootloader_info.h you find this:

#define BOOTLOADER_DFU_GPREGRET_MASK            (0xB0)      /**< Magic pattern written to GPREGRET register to signal between main app and DFU. The 3 lower bits are assumed to be used for signalling purposes.*/
#define BOOTLOADER_DFU_START_BIT_MASK           (0x01)      /**< Bit mask to signal from main application to enter DFU mode using a buttonless service. */

As I understand the comment, B0 is used as a kind of signature to say "yes, the register content is related to bootloader", and reading the 3 lower bits are used to "signal", bit 0 (lowest bit) is used to signal DFU should start.

I wanted to create my own signal, for which I use bit 1. So at some point, I set it, which gives the register a value of B2.

Now let's have a look at code in dfu_enter_flags_clears:

if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
   (nrf_power_gpregret_get() & BOOTLOADER_DFU_START))
{
    // Clear DFU mark in GPREGRET register.
    nrf_power_gpregret_set(nrf_power_gpregret_get() & ~BOOTLOADER_DFU_START);
}

This code will be executed as B2 & B1 != 0... Shouldn't it be replaced with:

if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
   ((nrf_power_gpregret_get() & 0xF8 == BOOTLOADER_DFU_GPREGRET_MASK) && (nrf_power_gpregret_get() & BOOTLOADER_DFU_START_BIT_MASK)))
{
    // Clear DFU mark in GPREGRET register.
    nrf_power_gpregret_set(nrf_power_gpregret_get() & ~BOOTLOADER_DFU_START);
}

Now let's have a look at this code from dfu_enter_check:

if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
   (nrf_power_gpregret_get() & BOOTLOADER_DFU_START))
{
    NRF_LOG_DEBUG("DFU mode requested via GPREGRET.");
    return true;
}

When GPRGRET == 0xB2, this code will get executed, which, as far as I understand, contradicts the documentation and the way it was intended to work (as BOOTLOADER_DFU_START_BIT_MASK is here unimportant, replacing BOOTLOADER_DFU_START with BOOTLOADER_DFU_GPREGRET_MASK would do the same job, while clearly ignoring the "signal flags", and then what's the point of having signal flags?)

So I guess I'm expecting 2 things:
- Confirmation that I understood properly the way "signal bits" were intended to be used.
- That case ID 211047 would not be regarded as a possible evolution, but rather as a bugfix, as the bootloader does not behave according to the way "described" by the documentation.

2.5.0.0

[tesc]

Hi,

I am sorry for the delay. Thank you very much, bug reports of this nature are always appreciated!

Looking into the matter I agree this is very likely to be a bug. Just as you write, the comments on the defines implies that there is a five bit magic pattern to signal it is a message from a buttonless DFU service to the DFU bootloader, and a three bit signal. But in dfu_enter_check, setting any of bits 7, 5, 4 or 0 will trigger DFU mode. This must be wrong.

I have updated the feature request that was registered based on the other thread, and it is now a bug report including your observations and my own. Hopefully it will be fixed before too long.

Regards,
Terje

[DerPMO]

This is a SERIOUS Problem! We've just stumbled on this by ourselves and you confirmed it here. To be honest, this kind of destroys my trust in the bootloader and the SDK because it seems like a basic lack of understanding binary operators.

Also, I expect that you do at least some sort of testing of such features and if you do, there is a major part missing:

In this case I suspect the devs checked that the correct magic byte indeed does trigger the DFU, and maybe even if 0x00 doesn't trigger it. But apparently nobody ever checked if any other byte could trigger this as well! I can only hope that Nordic recognizes this problem and adds something to the standard testing definitions to also check for "false positives" not only true positives and false negatives!

[tesc]

Hi,

You are entirely correct. In this location, the same approach to testing magic word + signal should be used as is correctly done in crc_on_valid_app_required() and in dfu_enter_flags_clear() in the same file. I have notified the SDK team.

Thank you for the reports.

Regards,
Terje

[David Meiklejohn]

Just a '+1' on this: I'm a bit annoyed that I spent half an afternoon debugging what I thought was an error in my code, to find that my bootloader was incorrectly entering DFU mode due to this bug.

I do understand that bugs happen - just a bit concerned that this was first reported more than 2 years ago, and still persists in SDK17, even though it's really easy to fix.

I'll need to make my own custom copy of nrfbootloader.c to fix this.  Oh well...

  

[tesc]

Hi,

+1 noted.

Can be fixed by changing nrf_bootloader.c lines 377-382:

    if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
       (nrf_power_gpregret_get() & BOOTLOADER_DFU_START))
    {
        NRF_LOG_DEBUG("DFU mode requested via GPREGRET.");
        return true;
    }

To this:

    if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
       (nrf_power_gpregret_get() & BOOTLOADER_DFU_GPREGRET_MASK) == BOOTLOADER_DFU_GPREGRET)
            && (nrf_power_gpregret_get() & BOOTLOADER_DFU_START_BIT_MASK))
    {
        NRF_LOG_DEBUG("DFU mode requested via GPREGRET.");
        return true;
    }

Regards,
Terje

[Pete W]

Caught me out as well.

Surely it should just simply use the start bit mask (0x01), i.e.

if (NRF_BL_DFU_ENTER_METHOD_GPREGRET &&
             (nrf_power_gpregret_get() & BOOTLOADER_DFU_START_BIT_MASK)
{
     NRF_LOG_DEBUG("DFU mode requested via GPREGRET.");
     return true;
}

[Pete W]

Ah, I get it now.