Skip to content

Commit

Permalink
nvs: hardening against bad sector close
Browse files Browse the repository at this point in the history 
A badly written sector close ate that has correct crc8 could allow
jumps outside the assigned flash area. This behavior is fixed.

A possibility existed that a badly erased sector or a incomplete write
of a large item created a empty closed sector. This has been fixed by:
a. Erase verification.
b. Clearing such a sector at startup.

Fixes #34382

Signed-off-by: Laczen JMS <laczenjms@gmail.com>

Co-Authered-by: Andrzej Puzdrowski <andrzej.puzdrowski@nordicsemi.no>
  • Loading branch information
@Laczen @carlescufi
Laczen authored and carlescufi committed May 6, 2021
1 parent af10c06 commit a71a8db
Showing 1 changed file with 110 additions and 49 deletions.
159 changes: 110 additions & 49 deletions subsys/fs/nvs/nvs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ static int nvs_flash_block_move(struct nvs_fs *fs, uint32_t addr, size_t len)
return 0;
}

/* erase a sector by first checking it is used and then erasing if required
/* erase a sector and verify erase was OK.
* return 0 if OK, errorcode on error.
*/
static int nvs_flash_erase_sector(struct nvs_fs *fs, uint32_t addr)
Expand All @@ -219,12 +219,6 @@ static int nvs_flash_erase_sector(struct nvs_fs *fs, uint32_t addr)
off_t offset;

addr &= ADDR_SECT_MASK;
rc = nvs_flash_cmp_const(fs, addr, fs->flash_parameters->erase_value,
fs->sector_size);
if (rc <= 0) {
/* flash error or empty sector */
return rc;
}

offset = fs->offset;
offset += fs->sector_size * (addr >> ADDR_SECT_SHIFT);
Expand All @@ -233,6 +227,15 @@ static int nvs_flash_erase_sector(struct nvs_fs *fs, uint32_t addr)
fs->sector_size);
rc = flash_erase(fs->flash_device, offset, fs->sector_size);

if (rc) {
return rc;
}

if (nvs_flash_cmp_const(fs, addr, fs->flash_parameters->erase_value,
fs->sector_size)) {
rc = -ENXIO;
}

return rc;
}

Expand Down Expand Up @@ -277,6 +280,48 @@ static int nvs_ate_cmp_const(const struct nvs_ate *entry, uint8_t value)
return 0;
}

/* nvs_ate_valid validates an ate:
* return 1 if crc8 and offset valid,
* 0 otherwise
*/
static int nvs_ate_valid(struct nvs_fs *fs, const struct nvs_ate *entry)
{
size_t ate_size;

ate_size = nvs_al_size(fs, sizeof(struct nvs_ate));

if ((nvs_ate_crc8_check(entry)) ||
(entry->offset >= (fs->sector_size - ate_size))) {
return 0;
}

return 1;

}

/* nvs_close_ate_valid validates an sector close ate: a valid sector close ate:
* - valid ate
* - len = 0 and id = 0xFFFF
* - offset points to location at ate multiple from sector size
* return 1 if valid, 0 otherwise
*/
static int nvs_close_ate_valid(struct nvs_fs *fs, const struct nvs_ate *entry)
{
size_t ate_size;

if ((!nvs_ate_valid(fs, entry)) || (entry->len != 0U) ||
(entry->id != 0xFFFF)) {
return 0;
}

ate_size = nvs_al_size(fs, sizeof(struct nvs_ate));
if ((fs->sector_size - entry->offset) % ate_size) {
return 0;
}

return 1;
}

/* store an entry in flash */
static int nvs_flash_wrt_entry(struct nvs_fs *fs, uint16_t id, const void *data,
size_t len)
Expand Down Expand Up @@ -307,7 +352,7 @@ static int nvs_flash_wrt_entry(struct nvs_fs *fs, uint16_t id, const void *data,
}
/* end of flash routines */

/* If the closing ate has an invalid crc8, its offset cannot be trusted and
/* If the closing ate is invalid, its offset cannot be trusted and
* the last valod ate of the sector should instead try to be recovered by going
* through all ate's.
*
Expand All @@ -334,7 +379,7 @@ static int nvs_recover_last_ate(struct nvs_fs *fs, uint32_t *addr)
if (rc) {
return rc;
}
if (!nvs_ate_crc8_check(&end_ate)) {
if (nvs_ate_valid(fs, &end_ate)) {
/* found a valid ate, update data_end_addr and *addr */
data_end_addr &= ADDR_SECT_MASK;
data_end_addr += end_ate.offset + end_ate.len;
Expand Down Expand Up @@ -386,20 +431,14 @@ static int nvs_prev_ate(struct nvs_fs *fs, uint32_t *addr, struct nvs_ate *ate)
return 0;
}

if (!nvs_ate_crc8_check(&close_ate)) {
/* update the address so it points to the last added ate.
* do a check on close_ate.offset so that it does not point
* outside a sector and is aligned to ate size.
*/
if (close_ate.offset < (fs->sector_size - ate_size) &&
!(close_ate.offset % ate_size)) {
(*addr) &= ADDR_SECT_MASK;
(*addr) += close_ate.offset;
return 0;
}
/* Update the address if the close ate is valid.
*/
if (nvs_close_ate_valid(fs, &close_ate)) {
(*addr) &= ADDR_SECT_MASK;
(*addr) += close_ate.offset;
return 0;
}
/* The close_ate had an invalid CRC8 or the last added ate offset was
* recognized as incorrect, `lets find out the last valid ate
/* The close_ate was invalid, `lets find out the last valid ate
* and point the address to this found ate.
*
* remark: if there was absolutely no valid data in the sector *addr
Expand Down Expand Up @@ -483,7 +522,7 @@ static int nvs_gc(struct nvs_fs *fs)

stop_addr = gc_addr - ate_size;

if (!nvs_ate_crc8_check(&close_ate)) {
if (nvs_close_ate_valid(fs, &close_ate)) {
gc_addr &= ADDR_SECT_MASK;
gc_addr += close_ate.offset;
} else {
Expand All @@ -500,7 +539,7 @@ static int nvs_gc(struct nvs_fs *fs)
return rc;
}

if (nvs_ate_crc8_check(&gc_ate)) {
if (!nvs_ate_valid(fs, &gc_ate)) {
continue;
}

Expand All @@ -517,7 +556,7 @@ static int nvs_gc(struct nvs_fs *fs)
* invalid, don't consider these as a match.
*/
if ((wlk_ate.id == gc_ate.id) &&
(!nvs_ate_crc8_check(&wlk_ate))) {
(nvs_ate_valid(fs, &wlk_ate))) {
break;
}
} while (wlk_addr != fs->ate_wra);
Expand Down Expand Up @@ -610,10 +649,21 @@ static int nvs_startup(struct nvs_fs *fs)
}
}

/* addr contains address of the last ate in the most recent sector
* search for the first ate containing all cells erased.
/* addr contains address of closing ate in the most recent sector,
* search for the last valid ate using the recover_last_ate routine
*/

rc = nvs_recover_last_ate(fs, &addr);
if (rc) {
goto end;
}


/* addr contains address of the last valid ate in the most recent sector
* search for the first ate containing all cells erased, in the process
* also update fs->data_wra.
*/
fs->ate_wra = addr - ate_size;
fs->ate_wra = addr;
fs->data_wra = addr & ADDR_SECT_MASK;

while (fs->ate_wra >= fs->data_wra) {
Expand All @@ -629,8 +679,8 @@ static int nvs_startup(struct nvs_fs *fs)
break;
}

if (!nvs_ate_crc8_check(&last_ate)) {
/* crc8 is ok, complete write of ate was performed */
if (nvs_ate_valid(fs, &last_ate)) {
/* complete write of ate was performed */
fs->data_wra = addr & ADDR_SECT_MASK;
/* Align the data write address to the current
* write block size so that it is possible to write to
Expand All @@ -653,22 +703,6 @@ static int nvs_startup(struct nvs_fs *fs)
fs->ate_wra -= ate_size;
}

/* possible data write after last ate write, update data_wra */
while (fs->ate_wra > fs->data_wra) {
empty_len = fs->ate_wra - fs->data_wra;

rc = nvs_flash_cmp_const(fs, fs->data_wra, erase_value,
empty_len);
if (rc < 0) {
goto end;
}
if (!rc) {
break;
}

fs->data_wra += fs->flash_parameters->write_block_size;
}

/* if the sector after the write sector is not empty gc was interrupted
* we need to restart gc, first erase the sector before restarting gc
* otherwise the data may not fit into the sector.
Expand All @@ -689,9 +723,36 @@ static int nvs_startup(struct nvs_fs *fs)
fs->ate_wra += (fs->sector_size - 2 * ate_size);
fs->data_wra = (fs->ate_wra & ADDR_SECT_MASK);
rc = nvs_gc(fs);
goto end;
}

/* possible data write after last ate write, update data_wra */
while (fs->ate_wra > fs->data_wra) {
empty_len = fs->ate_wra - fs->data_wra;

rc = nvs_flash_cmp_const(fs, fs->data_wra, erase_value,
empty_len);
if (rc < 0) {
goto end;
}
if (!rc) {
break;
}

fs->data_wra += fs->flash_parameters->write_block_size;
}

/* If the ate_wra is pointing to the first ate write location in a
* sector and data_wra is not 0, erase the sector as it contains no
* valid data (this also avoids closing a sector without any data).
*/
if (((fs->ate_wra + 2 * ate_size) == fs->sector_size) &&
(fs->data_wra != (fs->ate_wra & ADDR_SECT_MASK))) {
rc = nvs_flash_erase_sector(fs, fs->ate_wra);
if (rc) {
goto end;
}
fs->data_wra = fs->ate_wra & ADDR_SECT_MASK;
}

end:
Expand Down Expand Up @@ -820,7 +881,7 @@ ssize_t nvs_write(struct nvs_fs *fs, uint16_t id, const void *data, size_t len)
if (rc) {
return rc;
}
if ((wlk_ate.id == id) && (!nvs_ate_crc8_check(&wlk_ate))) {
if ((wlk_ate.id == id) && (nvs_ate_valid(fs, &wlk_ate))) {
prev_found = true;
break;
}
Expand Down Expand Up @@ -938,7 +999,7 @@ ssize_t nvs_read_hist(struct nvs_fs *fs, uint16_t id, void *data, size_t len,
if (rc) {
goto err;
}
if ((wlk_ate.id == id) && (!nvs_ate_crc8_check(&wlk_ate))) {
if ((wlk_ate.id == id) && (nvs_ate_valid(fs, &wlk_ate))) {
cnt_his++;
}
if (wlk_addr == fs->ate_wra) {
Expand Down Expand Up @@ -1014,7 +1075,7 @@ ssize_t nvs_calc_free_space(struct nvs_fs *fs)
}

if ((wlk_addr == step_addr) && step_ate.len &&
(!nvs_ate_crc8_check(&step_ate))) {
(nvs_ate_valid(fs, &step_ate))) {
/* count needed */
free_space -= nvs_al_size(fs, step_ate.len);
free_space -= ate_size;
Expand Down

0 comments on commit a71a8db

Please sign in to comment.