Base address gets added twice when checking for a valid erase in SlicingBlockDevice

[Jaco-Liebenberg]

Description of defect

I have written the following example program to demonstrate the bug.
Example program:

#include "mbed.h"
#include "FlashIAPBlockDevice.h"

static inline uint32_t align_up(uint64_t val, uint64_t size)
{
    return (((val - 1) / size) + 1) * size;
}

int get_flashiap_bd_addresses(bd_addr_t *start_address)
{
    int ret = MBED_SUCCESS;

    FlashIAP flash;
    static const int STORE_SECTORS = 4;

    flash.init();

    // Lets work from end of the flash backwards
    bd_addr_t curr_addr = flash.get_flash_start() + flash.get_flash_size();

    for (int i = STORE_SECTORS; i; i--) {
        bd_size_t sector_size = flash.get_sector_size(curr_addr - 1);
        curr_addr -= sector_size;
    }

    // bd and application-sectors mustn't overlap
    uint32_t first_wrtbl_sector_addr =
        (uint32_t)(align_up(FLASHIAP_APP_ROM_END_ADDR, flash.get_sector_size(FLASHIAP_APP_ROM_END_ADDR)));

    MBED_ASSERT(curr_addr >= first_wrtbl_sector_addr);
    if (curr_addr < first_wrtbl_sector_addr) {
        ret = MBED_ERROR_MEDIA_FULL;
    } else {
        *start_address = curr_addr;
    }

    flash.deinit();

    return ret;
}

BlockDevice *get_flash_bd(bd_addr_t start_address, bd_size_t *size)
{
    bd_addr_t flash_end_address;
    bd_addr_t flash_start_address;
    FlashIAP flash;

    int ret = flash.init();

    //Get flash parameters before starting
    flash_start_address = flash.get_flash_start();
    flash_end_address = flash_start_address + flash.get_flash_size();
    flash.deinit();

    //The block device will have all space from start address to the end of the flash
    *size = (flash_end_address - start_address);
    static FlashIAPBlockDevice bd(start_address, *size);

    return &bd;
}

int main(void)
{
    bd_size_t internal_size = 0;
    bd_addr_t internal_start_address = 0;
    int ret;

    get_flashiap_bd_addresses(&internal_start_address);    

    //Get internal memory FLASHIAP block device.
    auto main_bd = get_flash_bd(internal_start_address, &internal_size);
    SlicingBlockDevice sliced_bd2(main_bd, (internal_size/2), internal_size);        //Only use second half of main_bd
    ret = sliced_bd2.init();        //This fails

}

This example creates a FlashIAPBlockDevice using the last 4 sectors of the internal flash memory. It then creates a SlicingBlockDevice using the second half of the FlashIAPBlockDevice. This example program was specifically written to demonstrate the bug. It was developed on top of mbed-os 5.14.1 using the gcc_arm compiler and tested on the nucleo_f429zi. The bug can be seen by running the example program above, stepping into ret = sliced_bd2.init(); and checking why MBED_ASSERT(is_valid_erase(_start, _stop - _start)); on line 61 of SlicingBlockDevice.cpp fails. The example exposes the bug as the bug manifests in SlicingBlockDevice::init but the same bug has been duplicated for SlicingBlockDevice::erase.

Let's dive into it:

Examen the following call stack:

with SlicingBlockDevice::init

int SlicingBlockDevice::init()
{
    int err = _bd->init();
    if (err) {
        return err;
    }

    bd_size_t size = _bd->size();

    // Calculate from_end values
    if (_start_from_end) {
        _start_from_end = false;
        _start = size - _start;
    }

    if (_stop_from_end) {
        _stop_from_end = false;
        _stop = size - _stop;
    }

    // Check that block addresses are valid
    MBED_ASSERT(is_valid_erase(_start, _stop - _start));

    return 0;
}

SlicingBlockDevice::is_valid_erase

bool SlicingBlockDevice::is_valid_erase(bd_addr_t addr, bd_size_t size) const
{
    return (
               addr % get_erase_size(addr) == 0 &&
               (addr + size) % get_erase_size(addr + size - 1) == 0 &&
               addr + size <= (this->size() + _start));
}

and SlicingBlockDevice:: get_erase_size

bd_size_t SlicingBlockDevice:: get_erase_size(bd_addr_t addr) const
{
    return _bd->get_erase_size(_start + addr);
}

On line 61 of SlicingBlockDevice.cpp, init() calls MBED_ASSERT(is_valid_erase(_start, _stop - _start)) which makes sense. We want to check if it is valid to erase the whole area of the SlicingBlockDevice. This will give us an indication of whether the start and end addresses given for where the SlicingBlockDevice must exist, in the underlining block device, is valid.
init() calls is_valid_erase with _start passed as addr. is_valid_erase in turn calls get_erase_size(addr) with addr still equal to _start. Lastly, get_erase_size calls _bd->get_erase_size(_start + addr) where addr is still equal to _start. So this eventually becomes _bd->get_erase_size(_start + _start) where it clearly should have been _bd->get_erase_size(_start). The same can be said for erase where it also calls is_valid_erase(addr + _start, size). This leads to the call to _bd->get_erase_size being equal to _bd->get_erase_size(_start + _start + addr) which clearly should be _bd->get_erase_size(_start + addr).

The quick and dirty solution to this is probably to change SlicingBlockDevice::get_erase_size to:

bd_size_t SlicingBlockDevice:: get_erase_size(bd_addr_t addr) const
{
    return _bd->get_erase_size( addr);
}

This will solve the problem in both the init and erase case but this is undesirable as SlicingBlockDevice::get_ease_size is public and it would require any other use of
SlicingBlockDevice::get_ease_size to provide addr relative to the underlining block device instead of providing addr relative to the SlicingBlockDevice. We can not (nor should we want to) expect the consumers of SlicingBlockDevice to know the semantics of where the SlicingBlockDevice exists in the underlining block device.

Another solution would be to change the MBED_ASSERT of init and erase to MBED_ASSERT(is_valid_erase(0, _stop - _start)); and MBED_ASSERT(is_valid_erase(addr, size)); respectively. This would solve the problem, but I would argue it is still not the best solution.

Looking at it a bit more holistically, the same argument can be made for is_valid_erase, is_valid_program and is_valid_read, as for get_erase_size. They are all three public functions of SlicingBlockDevice that can be accessed by any consumer of SlicingBlockDevice. Those consumers should not need to know the semantics of where the SlicingBlockDevice exists in the underlining BlockDevice yet examining how they are used in read, program and erase

int SlicingBlockDevice::read(void *b, bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_read(addr + _start, size));
    return _bd->read(b, addr + _start, size);
}

int SlicingBlockDevice::program(const void *b, bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_program(addr + _start, size));
    return _bd->program(b, addr + _start, size);
}

int SlicingBlockDevice::erase(bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_erase(addr + _start, size));
    return _bd->erase(addr + _start, size);
}

it would seem the intent was for them to be used with the address supplied to these functions being given relative to the underlining BlockDevice instead of it being relative to the SlicingBlockDevice.

The only limitations placed on these operations (read, program and erase) by the SlicingBlockDevice is ensuring that these operations stay within the bounds of the SlicingBlockDevice (stay between _start and _end). All other limitations placed upon these operations are limitations of the underlining BlockDevice. Why not use the respective calls to the underlining BlockDivice to determine if it is a valid operation according to the underlining BlockDevice and then additionally just test whether or not the given operation will stay within the bounds of the SlicingBlockDevice?

My suggested solution would thus be to change is_valid_read, is_valid_program and is_valid_erase as follows:

bool SlicingBlockDevice::is_valid_read(bd_addr_t addr, bd_size_t size) const
{
    return (
                _bd->is_valid_read(addr + _start, size) &&      //Test that this is a valid operation of the underlining BlockDevice
                addr + size <= this->size());                   //Test that this operation will stay within the bounds of the SlicingBlockDevice
}

bool SlicingBlockDevice::is_valid_program(bd_addr_t addr, bd_size_t size) const
{
    return (
               _bd->is_valid_program(addr + _start, size) &&      //Test that this is a valid operation of the underlining BlockDevice
                addr + size <= this->size());                     //Test that this operation will stay within the bounds of the SlicingBlockDevice
}

bool SlicingBlockDevice::is_valid_erase(bd_addr_t addr, bd_size_t size) const
{
    return (
               _bd->is_valid_erase(addr + _start, size) &&      //Test that this is a valid operation of the underlining BlockDevice
                addr + size <= this->size());                   //Test that this operation will stay within the bounds of the SlicingBlockDevice
}

read, program and erase will thus accordingly change to:

int SlicingBlockDevice::read(void *b, bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_read(addr, size));
    return _bd->read(b, addr + _start, size);
}

int SlicingBlockDevice::program(const void *b, bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_program(addr, size));
    return _bd->program(b, addr + _start, size);
}

int SlicingBlockDevice::erase(bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_erase(addr, size));
    return _bd->erase(addr + _start, size);
}

And the MBED_ASSERT in init should still be changed to MBED_ASSERT(is_valid_erase(0, _stop - _start))

This will allow any consumer of SlicingBlockDevice to call any of those methods while being blissfully unaware of the semantics to the underlining BlockDevice. Should there exist any other limitations within the underlining BlockDevice, qualifying the respective operations against those limitations will now be left to the underlining BlockDevice.

Target(s) affected by this defect ?

This bug was picked up using the nucleo_f429zi but should affect all targets.

Toolchain(s) (name and version) displaying this defect ?

The bug was picked up using gcc_arm version 8.2.1 but should affect all toolchains.

What version of Mbed-os are you using (tag or sha) ?

mbed-os-5.14.1, 679d248

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

mbed-cli version 1.10.1

How is this defect reproduced ?

This defect can be reproduced by running the example program given above or by initializing any SlicingBlockDevice where the SlicingBlockDevice does not reside at the start of the underlining BlockDevice.

[0xc0170]

Thanks for the detailed report, we will review

cc @ARMmbed/mbed-os-storage

[ciarmcom]

Internal Jira reference: https://jira.arm.com/browse/MBOTRIAGE-2304

[geoffrey-vl]

Does this impact all mbed-os versions <mbed-os 5.14.1?

[0xc0170]

The fix went to 5.14.2 so anything before (<5.14.2) could be. @geoffrey-vl Are you having the same issue with older versions?

[geoffrey-vl]

Well I'm not sure yet, we've seen a is_valid_read(addr, size) error in one of our production devices this week. We're running mbed-os 5.9.2. The SlicingBlockDevice doesn't yet override is_valid_erase() in this os version.
We have following implementation for slicingblock device:

...

int SlicingBlockDevice::erase(bd_addr_t addr, bd_size_t size)
{
    MBED_ASSERT(is_valid_erase(addr, size));
    return _bd->erase(addr + _start, size);
}

bd_size_t SlicingBlockDevice::get_read_size() const
{
    return _bd->get_read_size();
}

bd_size_t SlicingBlockDevice::get_program_size() const
{
    return _bd->get_program_size();
}

bd_size_t SlicingBlockDevice::get_erase_size() const
{
    return _bd->get_erase_size();
}

bd_size_t SlicingBlockDevice::get_erase_size(bd_addr_t addr) const
{
    return _bd->get_erase_size(_start + addr);
}

int SlicingBlockDevice::get_erase_value() const
{
    return _bd->get_erase_value();
}

bd_size_t SlicingBlockDevice::size() const
{
    return _stop - _start;
}

What's is the effect of the bug described above? In our case we're editing a file, but at one point FAT tries to allocate behind our slicingblockdevice border. But in our case I guess the bug is due to a misalignment in FAT compared to the slicing block device. We had the FAT partition created in an earlier firmware version where there was no slicingblockdevice, hence FAT thinks it can use the entire disk instead of just that slice.