From f1c430e779b81d891e9ce7fa7ea5acbcbc98d312 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 24 Sep 2024 13:31:23 -0500 Subject: [PATCH 1/2] Added some tests around seek integer overflow/underflow Original tests provided by m-kostrzewa, these identify signed overflow (undefined behavior) when compiled with -fsanitize=undefined. --- tests/test_seek.toml | 108 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/tests/test_seek.toml b/tests/test_seek.toml index 9b3768d7c..33fb57859 100644 --- a/tests/test_seek.toml +++ b/tests/test_seek.toml @@ -405,3 +405,111 @@ code = ''' lfs_file_close(&lfs, &file) => 0; lfs_unmount(&lfs) => 0; ''' + + +# test possible overflow/underflow conditions +# +# note these need -fsanitize=undefined to consistently detect +# overflow/underflow conditions + +[cases.test_seek_filemax] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + lfs_file_t file; + lfs_file_open(&lfs, &file, "kitty", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0; + uint8_t buffer[1024]; + strcpy((char*)buffer, "kittycatcat"); + size_t size = strlen((char*)buffer); + lfs_file_write(&lfs, &file, buffer, size) => size; + + // seek with LFS_SEEK_SET + lfs_file_seek(&lfs, &file, LFS_FILE_MAX, LFS_SEEK_SET) => LFS_FILE_MAX; + + // seek with LFS_SEEK_CUR + lfs_file_seek(&lfs, &file, 0, LFS_SEEK_CUR) => LFS_FILE_MAX; + + // the file hasn't changed size, so seek end takes us back to the offset=0 + lfs_file_seek(&lfs, &file, +10, LFS_SEEK_END) => size+10; + + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; +''' + +[cases.test_seek_underflow] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + lfs_file_t file; + lfs_file_open(&lfs, &file, "kitty", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0; + uint8_t buffer[1024]; + strcpy((char*)buffer, "kittycatcat"); + size_t size = strlen((char*)buffer); + lfs_file_write(&lfs, &file, buffer, size) => size; + + // underflow with LFS_SEEK_CUR, should error + lfs_file_seek(&lfs, &file, -(size+10), LFS_SEEK_CUR) => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, -LFS_FILE_MAX, LFS_SEEK_CUR) => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, -(size+LFS_FILE_MAX), LFS_SEEK_CUR) + => LFS_ERR_INVAL; + + // underflow with LFS_SEEK_END, should error + lfs_file_seek(&lfs, &file, -(size+10), LFS_SEEK_END) => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, -LFS_FILE_MAX, LFS_SEEK_END) => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, -(size+LFS_FILE_MAX), LFS_SEEK_END) + => LFS_ERR_INVAL; + + // file pointer should not have changed + lfs_file_tell(&lfs, &file) => size; + + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; +''' + +[cases.test_seek_overflow] +code = ''' + lfs_t lfs; + lfs_format(&lfs, cfg) => 0; + lfs_mount(&lfs, cfg) => 0; + lfs_file_t file; + lfs_file_open(&lfs, &file, "kitty", + LFS_O_WRONLY | LFS_O_CREAT | LFS_O_APPEND) => 0; + uint8_t buffer[1024]; + strcpy((char*)buffer, "kittycatcat"); + size_t size = strlen((char*)buffer); + lfs_file_write(&lfs, &file, buffer, size) => size; + + // seek to LFS_FILE_MAX + lfs_file_seek(&lfs, &file, LFS_FILE_MAX, LFS_SEEK_SET) => LFS_FILE_MAX; + + // overflow with LFS_SEEK_CUR, should error + lfs_file_seek(&lfs, &file, +10, LFS_SEEK_CUR) => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, +LFS_FILE_MAX, LFS_SEEK_CUR) => LFS_ERR_INVAL; + + // LFS_SEEK_SET/END don't care about the current file position, but we can + // still overflow with a large offset + + // overflow with LFS_SEEK_SET, should error + lfs_file_seek(&lfs, &file, + +((uint32_t)LFS_FILE_MAX+10), + LFS_SEEK_SET) => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, + +((uint32_t)LFS_FILE_MAX+(uint32_t)LFS_FILE_MAX), + LFS_SEEK_SET) => LFS_ERR_INVAL; + + // overflow with LFS_SEEK_END, should error + lfs_file_seek(&lfs, &file, +(LFS_FILE_MAX-size+10), LFS_SEEK_END) + => LFS_ERR_INVAL; + lfs_file_seek(&lfs, &file, +(LFS_FILE_MAX-size+LFS_FILE_MAX), LFS_SEEK_END) + => LFS_ERR_INVAL; + + // file pointer should not have changed + lfs_file_tell(&lfs, &file) => LFS_FILE_MAX; + + lfs_file_close(&lfs, &file) => 0; + lfs_unmount(&lfs) => 0; +''' From abaec45652a1e8c79f812c3e868f6c1063e06c51 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 24 Sep 2024 13:47:28 -0500 Subject: [PATCH 2/2] Fixed seek undefined behavior on signed integer overflow In the previous implementation of lfs_file_seek, we calculated the new offset using signed arithmetic before checking for possible overflow/underflow conditions. This results in undefined behavior in C. Fortunately for us, littlefs is now limited to 31-bit file sizes for API reasons, so we don't have to be too clever here. Doing the arithmetic with unsigned integers and just checking if we're in a valid range afterwards should work. Found by m-kostrzewa and lucic71 --- lfs.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/lfs.c b/lfs.c index d35d5d6db..933f69c70 100644 --- a/lfs.c +++ b/lfs.c @@ -3664,22 +3664,16 @@ static lfs_ssize_t lfs_file_write_(lfs_t *lfs, lfs_file_t *file, static lfs_soff_t lfs_file_seek_(lfs_t *lfs, lfs_file_t *file, lfs_soff_t off, int whence) { // find new pos + // + // fortunately for us, littlefs is limited to 31-bit file sizes, so we + // don't have to worry too much about integer overflow lfs_off_t npos = file->pos; if (whence == LFS_SEEK_SET) { npos = off; } else if (whence == LFS_SEEK_CUR) { - if ((lfs_soff_t)file->pos + off < 0) { - return LFS_ERR_INVAL; - } else { - npos = file->pos + off; - } + npos = file->pos + (lfs_off_t)off; } else if (whence == LFS_SEEK_END) { - lfs_soff_t res = lfs_file_size_(lfs, file) + off; - if (res < 0) { - return LFS_ERR_INVAL; - } else { - npos = res; - } + npos = (lfs_off_t)lfs_file_size_(lfs, file) + (lfs_off_t)off; } if (npos > lfs->file_max) {