Serial recovery decrypt_region_inplace() uses variable length arrays

[stindaNXP]

static int
decrypt_region_inplace(struct enc_key_data  *enc_data,
                       const struct flash_area *fap,
                       struct image_header *hdr,
                       uint32_t off, uint32_t sz)
{
    uint32_t bytes_copied;
    int chunk_sz;
    int rc;
    uint32_t tlv_off;
    size_t blk_off;
    uint16_t idx;
    uint32_t blk_sz;
    uint8_t buf[sz] __attribute__((aligned)); //<<<<<<<<<<<<<<<<<<<
    assert(sz <= sizeof buf);                 //<<<<<<<<<<<<<<<<<<<

There are several problems with this approach. IAR and Keil doesn't enable variable length arrays by default, so it compiles with the error "Error[Pe028]: expression must have a constant value". It is also forbidden by several safety standarts like MISRA.

The sz argument should match sector size which leads to stack overflow risk. If you consider that there are flashless devices what could use a flash memory with sector size like 256kB...

assert check is not enough as it is valid only during development state.

I think it woul be a good idea to rewrite it to static buffer with a size of flash page rather than a sector.

[nordicjm]

The sz argument should match sector size which leads to stack overflow risk. If you consider that there are flashless devices what could use a flash memory with sector size like 256kB...

I assume these devices have > 300KiB RAM?

assert check is not enough as it is valid only during development state.

But that is an assert doing it's job, right? You do a developer build, test things like firmware updating, if it asserts then you see something is not right, this is not a check that should happen at runtime for all builds since it's completely pointless once you've verified the configuration values are sufficient

I think it woul be a good idea to rewrite it to static buffer with a size of flash page rather than a sector.

But you can't do that because you only know the sector size of a partition at run time, which actually depending upon what the current sector is since some devices have flash areas with multiple sector sizes, so how would one account for that?

[stindaNXP]

Hello Jamie

The sz argument should match sector size which leads to stack overflow risk. If you consider that there are flashless devices what could use a flash memory with sector size like 256kB...

I assume these devices have > 300KiB RAM?

Not always. Especially in cost-sensitive applications.

assert check is not enough as it is valid only during development state.

But that is an assert doing it's job, right? You do a developer build, test things like firmware updating, if it asserts then you see something is not right, this is not a check that should happen at runtime for all builds since it's completely pointless once you've verified the configuration values are sufficient

ok

I think it woul be a good idea to rewrite it to static buffer with a size of flash page rather than a sector.

But you can't do that because you only know the sector size of a partition at run time, which actually depending upon what the current sector is since some devices have flash areas with multiple sector sizes, so how would one account for that?

This depends on how the user implements their flash backend. The page size could be obtained by a function called flash_area_get_page, similar to flash_area_get_sector. For simplicity, users should place the image slot in a flash area without transitions between regions with different sector sizes.

Anyway, I think the main point here is that the VLA is not compliant by safety standarts.

[nordicjm]

Hello Jamie

The sz argument should match sector size which leads to stack overflow risk. If you consider that there are flashless devices what could use a flash memory with sector size like 256kB...

I assume these devices have > 300KiB RAM?

Not always. Especially in cost-sensitive applications.

Then encryption is not going to work

assert check is not enough as it is valid only during development state.

But that is an assert doing it's job, right? You do a developer build, test things like firmware updating, if it asserts then you see something is not right, this is not a check that should happen at runtime for all builds since it's completely pointless once you've verified the configuration values are sufficient

ok

I think it woul be a good idea to rewrite it to static buffer with a size of flash page rather than a sector.

But you can't do that because you only know the sector size of a partition at run time, which actually depending upon what the current sector is since some devices have flash areas with multiple sector sizes, so how would one account for that?

This depends on how the user implements their flash backend. The page size could be obtained by a function called flash_area_get_page, similar to flash_area_get_sector. For simplicity, users should place the image slot in a flash area without transitions between regions with different sector sizes.

That's not a valid requirement, sorry, devices with variable sector sizes are supported and that is by design. The only way to support this would be to use malloc instead, and I don't think that's any better because now suddenly you need an C library implementation for that

Anyway, I think the main point here is that the VLA is not compliant by safety standarts.

Agreed, if it's possible to replace it then it would be good, but it cannot lead to a loss in any functionality and should not require a user setting a config to specify the maximum size

[stindaNXP]

I got your points.

The decryption could be done by SBL for SWAP and OVERWRITE_ONLY upgrade mode as usual during boot, but in the single slot case this is the only way unfortunately as you need decryption keys in TLV.

This issue can be closed.

[stindaNXP]

Hi @nordicjm

I think an alternative approach could use a move algorithm. The encryption flag can be detected before the first write to flash, so an encrypted image could be placed at an offset aligned to the flash sector size and then decrypted with smaller static buffer. Since the user must reserve a flash sector for the slot trailer anyway, this won't overwrite the second slot.

What do you think?