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

@@ -2128,14 +2128,13 @@ static int lfs_dir_splittingcompact(lfs_t *lfs, lfs_mdir_t *dir,
             // And we cap at half a block to avoid degenerate cases with
             // nearly-full metadata blocks.
             //
-            lfs_size_t metadata_max = (lfs->cfg->metadata_max)
-                    ? lfs->cfg->metadata_max
-                    : lfs->cfg->block_size;
             if (end - split < 0xff
                     && size <= lfs_min(
-                        metadata_max - 40,
+                        lfs->cfg->block_size - 40,
                         lfs_alignup(
-                            metadata_max/2,
+                            (lfs->cfg->metadata_max
+                                ? lfs->cfg->metadata_max
+                                : lfs->cfg->block_size)/2,
                             lfs->cfg->prog_size))) {
                 break;
             }
@@ -3665,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) {
@@ -4210,15 +4203,6 @@ static int lfs_init(lfs_t *lfs, const struct lfs_config *cfg) {
     LFS_ASSERT(lfs->cfg->compact_thresh == (lfs_size_t)-1
             || lfs->cfg->compact_thresh <= lfs->cfg->block_size);
 
-    // check that metadata_max is a multiple of read_size and prog_size,
-    // and a factor of the block_size
-    LFS_ASSERT(!lfs->cfg->metadata_max
-            || lfs->cfg->metadata_max % lfs->cfg->read_size == 0);
-    LFS_ASSERT(!lfs->cfg->metadata_max
-            || lfs->cfg->metadata_max % lfs->cfg->prog_size == 0);
-    LFS_ASSERT(!lfs->cfg->metadata_max
-            || lfs->cfg->block_size % lfs->cfg->metadata_max == 0);
-
     // setup read cache
     if (lfs->cfg->read_buffer) {
         lfs->rcache.buffer = lfs->cfg->read_buffer;

runners/bench_runner.c

@@ -1322,7 +1322,6 @@ void perm_run(
         .cache_size         = CACHE_SIZE,
         .lookahead_size     = LOOKAHEAD_SIZE,
         .compact_thresh     = COMPACT_THRESH,
-        .metadata_max       = METADATA_MAX,
         .inline_max         = INLINE_MAX,
     };
 

runners/bench_runner.h

@@ -96,13 +96,12 @@ intmax_t bench_define(size_t define);
 #define CACHE_SIZE_i         6
 #define LOOKAHEAD_SIZE_i     7
 #define COMPACT_THRESH_i     8
-#define METADATA_MAX_i       9
-#define INLINE_MAX_i         10
-#define BLOCK_CYCLES_i       11
-#define ERASE_VALUE_i        12
-#define ERASE_CYCLES_i       13
-#define BADBLOCK_BEHAVIOR_i  14
-#define POWERLOSS_BEHAVIOR_i 15
+#define INLINE_MAX_i         9
+#define BLOCK_CYCLES_i       10
+#define ERASE_VALUE_i        11
+#define ERASE_CYCLES_i       12
+#define BADBLOCK_BEHAVIOR_i  13
+#define POWERLOSS_BEHAVIOR_i 14
 
 #define READ_SIZE           bench_define(READ_SIZE_i)
 #define PROG_SIZE           bench_define(PROG_SIZE_i)
@@ -113,7 +112,6 @@ intmax_t bench_define(size_t define);
 #define CACHE_SIZE          bench_define(CACHE_SIZE_i)
 #define LOOKAHEAD_SIZE      bench_define(LOOKAHEAD_SIZE_i)
 #define COMPACT_THRESH      bench_define(COMPACT_THRESH_i)
-#define METADATA_MAX        bench_define(METADATA_MAX_i)
 #define INLINE_MAX          bench_define(INLINE_MAX_i)
 #define BLOCK_CYCLES        bench_define(BLOCK_CYCLES_i)
 #define ERASE_VALUE         bench_define(ERASE_VALUE_i)
@@ -131,7 +129,6 @@ intmax_t bench_define(size_t define);
     BENCH_DEF(CACHE_SIZE,         lfs_max(64,lfs_max(READ_SIZE,PROG_SIZE))) \
     BENCH_DEF(LOOKAHEAD_SIZE,     16) \
     BENCH_DEF(COMPACT_THRESH,     0) \
-    BENCH_DEF(METADATA_MAX,       0) \
     BENCH_DEF(INLINE_MAX,         0) \
     BENCH_DEF(BLOCK_CYCLES,       -1) \
     BENCH_DEF(ERASE_VALUE,        0xff) \
@@ -140,7 +137,7 @@ intmax_t bench_define(size_t define);
     BENCH_DEF(POWERLOSS_BEHAVIOR, LFS_EMUBD_POWERLOSS_NOOP)
 
 #define BENCH_GEOMETRY_DEFINE_COUNT 4
-#define BENCH_IMPLICIT_DEFINE_COUNT 16
+#define BENCH_IMPLICIT_DEFINE_COUNT 15
 
 
 #endif

runners/test_runner.c

@@ -1347,7 +1347,6 @@ static void run_powerloss_none(
         .cache_size         = CACHE_SIZE,
         .lookahead_size     = LOOKAHEAD_SIZE,
         .compact_thresh     = COMPACT_THRESH,
-        .metadata_max       = METADATA_MAX,
         .inline_max         = INLINE_MAX,
     #ifdef LFS_MULTIVERSION
         .disk_version       = DISK_VERSION,
@@ -1426,7 +1425,6 @@ static void run_powerloss_linear(
         .cache_size         = CACHE_SIZE,
         .lookahead_size     = LOOKAHEAD_SIZE,
         .compact_thresh     = COMPACT_THRESH,
-        .metadata_max       = METADATA_MAX,
         .inline_max         = INLINE_MAX,
     #ifdef LFS_MULTIVERSION
         .disk_version       = DISK_VERSION,
@@ -1522,7 +1520,6 @@ static void run_powerloss_log(
         .cache_size         = CACHE_SIZE,
         .lookahead_size     = LOOKAHEAD_SIZE,
         .compact_thresh     = COMPACT_THRESH,
-        .metadata_max       = METADATA_MAX,
         .inline_max         = INLINE_MAX,
     #ifdef LFS_MULTIVERSION
         .disk_version       = DISK_VERSION,
@@ -1616,7 +1613,6 @@ static void run_powerloss_cycles(
         .cache_size         = CACHE_SIZE,
         .lookahead_size     = LOOKAHEAD_SIZE,
         .compact_thresh     = COMPACT_THRESH,
-        .metadata_max       = METADATA_MAX,
         .inline_max         = INLINE_MAX,
     #ifdef LFS_MULTIVERSION
         .disk_version       = DISK_VERSION,
@@ -1808,7 +1804,6 @@ static void run_powerloss_exhaustive(
         .cache_size         = CACHE_SIZE,
         .lookahead_size     = LOOKAHEAD_SIZE,
         .compact_thresh     = COMPACT_THRESH,
-        .metadata_max       = METADATA_MAX,
         .inline_max         = INLINE_MAX,
     #ifdef LFS_MULTIVERSION
         .disk_version       = DISK_VERSION,

runners/test_runner.h

@@ -89,14 +89,13 @@ intmax_t test_define(size_t define);
 #define CACHE_SIZE_i         6
 #define LOOKAHEAD_SIZE_i     7
 #define COMPACT_THRESH_i     8
-#define METADATA_MAX_i       9
-#define INLINE_MAX_i         10
-#define BLOCK_CYCLES_i       11
-#define ERASE_VALUE_i        12
-#define ERASE_CYCLES_i       13
-#define BADBLOCK_BEHAVIOR_i  14
-#define POWERLOSS_BEHAVIOR_i 15
-#define DISK_VERSION_i       16
+#define INLINE_MAX_i         9
+#define BLOCK_CYCLES_i       10
+#define ERASE_VALUE_i        11
+#define ERASE_CYCLES_i       12
+#define BADBLOCK_BEHAVIOR_i  13
+#define POWERLOSS_BEHAVIOR_i 14
+#define DISK_VERSION_i       15
 
 #define READ_SIZE           TEST_DEFINE(READ_SIZE_i)
 #define PROG_SIZE           TEST_DEFINE(PROG_SIZE_i)
@@ -107,7 +106,6 @@ intmax_t test_define(size_t define);
 #define CACHE_SIZE          TEST_DEFINE(CACHE_SIZE_i)
 #define LOOKAHEAD_SIZE      TEST_DEFINE(LOOKAHEAD_SIZE_i)
 #define COMPACT_THRESH      TEST_DEFINE(COMPACT_THRESH_i)
-#define METADATA_MAX        TEST_DEFINE(METADATA_MAX_i)
 #define INLINE_MAX          TEST_DEFINE(INLINE_MAX_i)
 #define BLOCK_CYCLES        TEST_DEFINE(BLOCK_CYCLES_i)
 #define ERASE_VALUE         TEST_DEFINE(ERASE_VALUE_i)
@@ -126,7 +124,6 @@ intmax_t test_define(size_t define);
     TEST_DEF(CACHE_SIZE,         lfs_max(64,lfs_max(READ_SIZE,PROG_SIZE))) \
     TEST_DEF(LOOKAHEAD_SIZE,     16) \
     TEST_DEF(COMPACT_THRESH,     0) \
-    TEST_DEF(METADATA_MAX,       0) \
     TEST_DEF(INLINE_MAX,         0) \
     TEST_DEF(BLOCK_CYCLES,       -1) \
     TEST_DEF(ERASE_VALUE,        0xff) \
@@ -136,7 +133,7 @@ intmax_t test_define(size_t define);
     TEST_DEF(DISK_VERSION,       0)
 
 #define TEST_GEOMETRY_DEFINE_COUNT 4
-#define TEST_IMPLICIT_DEFINE_COUNT 17
+#define TEST_IMPLICIT_DEFINE_COUNT 16
 
 
 #endif

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;
+'''

tests/test_superblocks.toml

@@ -523,30 +523,3 @@ code = '''
     assert(memcmp(buffer, "hello!", 6) == 0);
     lfs_unmount(&lfs) => 0;
 '''
-
-# test that metadata_max does not cause problems for superblock compaction
-[cases.test_superblocks_metadata_max]
-defines.METADATA_MAX = [
-    'lfs_max(512, PROG_SIZE)',
-    'lfs_max(BLOCK_SIZE/2, PROG_SIZE)',
-    'BLOCK_SIZE'
-]
-defines.N = [10, 100, 1000]
-code = '''
-    lfs_t lfs;
-    lfs_format(&lfs, cfg) => 0;
-    lfs_mount(&lfs, cfg) => 0;
-    for (int i = 0; i < N; i++) {
-        lfs_file_t file;
-        char name[256];
-        sprintf(name, "hello%03x", i);
-        lfs_file_open(&lfs, &file, name,
-                LFS_O_WRONLY | LFS_O_CREAT | LFS_O_EXCL) => 0;
-        lfs_file_close(&lfs, &file) => 0;
-        struct lfs_info info;
-        lfs_stat(&lfs, name, &info) => 0;
-        assert(strcmp(info.name, name) == 0);
-        assert(info.type == LFS_TYPE_REG);
-    }
-    lfs_unmount(&lfs) => 0;
-'''