DFU bootloader - CRC always 0, always accepted

[Erik]

Hi all, in the forum I can read about people having issues with wrong CRC in the bootloader, but with SDK11; I have the opposite problem. It is always 0 and is always accepted. If I program my application using the bootloader, then flash something else to the application region using IAR, the new "faulty" application is accepted and started anyway.

Is there a reason for why CRC checking has been removed from each boot or is this a bug? (It was there for single_bank in SDK7 at least)

bootloader.c:

bool bootloader_app_is_valid(uint32_t app_addr)
//...
// A stored crc value of 0 indicates that CRC checking is not used.
    if (p_bootloader_settings->bank_0_crc != 0)
    {
        image_crc = crc16_compute((uint8_t *)DFU_BANK_0_REGION_START,
                                  p_bootloader_settings->bank_0_size,
                                  NULL);
    }

Looking at SDK7 single_bank.c I can see that the code in dfu_image_validate has changed from

        else
        {
            // Valid peer activity detected. Hence restart the DFU timer.
            err_code = dfu_timer_restart();
            
            // Calculate CRC from DFU_BANK_0_REGION_START to mp_app_write_address.
            m_image_crc  = crc16_compute((uint8_t*)DFU_BANK_0_REGION_START, 
                                         m_image_size, 
                                         NULL);
            received_crc = uint16_decode((uint8_t*)&m_init_packet[0]);
                
            if ((m_init_packet_length != 0) && (m_image_crc != received_crc))
            {
                return NRF_ERROR_INVALID_DATA;
            }
            m_dfu_state = DFU_STATE_WAIT_4_ACTIVATE;
            err_code    = NRF_SUCCESS;
        }

to (dual_bank in SDK11):

        else
        {
            m_dfu_state = DFU_STATE_VALIDATE;

            // Valid peer activity detected. Hence restart the DFU timer.
            err_code = dfu_timer_restart();
            if (err_code == NRF_SUCCESS)
            {
                err_code = dfu_init_postvalidate((uint8_t *)mp_storage_handle_active->block_id,
                                                 m_image_size);
                VERIFY_SUCCESS(err_code);

                m_dfu_state = DFU_STATE_WAIT_4_ACTIVATE;
            }
        }

Now the dfu_init_postvalidate(...) only compares the CRC once, it does not update the m_image_crc in dfu_dual_bank.c, thus the m_image_crc is always 0 and bootloader_dfu_update_process(..) is always called with .app_crc = 0, causing the bootloader setting to be stored with an app CRC = 0.

My solution was to make dfu_init_postvalidate() return the calculated CRC through a pointer and then update m_image_crc. This seems to work for me, and now the CRC is verified each bootup.

EDIT: I sent a reference to m_image_crc to dfu_init_postvalidate in dfu_dual_bank line 585):

                    err_code = dfu_init_postvalidate((uint8_t *)mp_storage_handle_active->block_id,
                                                 m_image_size,
                                                 &m_image_crc);

Then in dfu_init_template.c (and dfu_init.h) I added a pointer as a function parameter and update it with the calculated CRC:

uint32_t dfu_init_postvalidate(uint8_t * p_image, uint32_t image_len, uint16_t* p_crc)
{
//...
// calculate CRC from active block.
image_crc = crc16_compute(p_image, image_len, NULL);
*p_crc = image_crc;
//...
}

/EDIT

A second opinon on my solution and a reason for why it has been removed would be nice. Also if this is intended, maybe the comment on m_image_crc could be updated to clearify that it is not in use.

Best Regards Erik

[Vidar Berg]

Hi,

The CRC check of the application image on boot is disabled in the newer SDK examples ( .bank_0_crc is just set to '0' ). This is done in case the application needs to modify its own image in flash at runtime where having a CRC check would have caused any following startups to fail.

But the application image will normally not be modified as user data is typically stored outside of the application image in the 'app data section'. So unless the appliation is writing to its own flash area I think makes sense to add CRC check on startup as an extra safety measure. @Erik's solution for adding it is correct.

[Erik]

Maybe a #define could enable/disable it, or at least it could be commented in the code that m_image_crc is not in use. Now it looks like it is implemented, but actually its not in use. Gives kind of a false security. Also an application modifying its own image is not very safe and I don't see why that sould be the default.