From 02f548f9ba2432b08d40d4d63fb38aeb150d9e5e Mon Sep 17 00:00:00 2001
From: Lucian Popescu <lucian.popescu187@gmail.com>
Date: Fri, 16 Aug 2024 18:04:41 +0300
Subject: [PATCH] Add stronger checks for offset in seek

Previous checks were trying to validate that file->pos does not become
negative. However, the method used for checking this contains possible
undefined behaivor (UB) because of the signed integer overflow.

This commit adds stronger checks for offset calculation:
  * make sure that ((lfs_soff_t) file->pos + off) is never < 0. Instead
    of using signed addition to check that (which can possibly lead to
    UB), use signed comparison: off < 0 && (lfs_soff_t) file->pos <
    -off. A special check of off against INT32_MIN is added to make sure
    that -off does not get transformed into -INT32_MIN, which is as well UB.
  * make sure that unsigned overflow does not occur in file->pos +
    (lfs_off_t) off.

Thoughts:
  * the lseek manual mandates an EOVERFLOW when the new offset cannot be
    represened in the offset type. I wonder if we want to return that
    instead of INVAL when an unsigned overflow occurs.
---
 lfs.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/lfs.c b/lfs.c
index d35d5d6d..c70f44b2 100644
--- a/lfs.c
+++ b/lfs.c
@@ -3661,6 +3661,26 @@ static lfs_ssize_t lfs_file_write_(lfs_t *lfs, lfs_file_t *file,
 }
 #endif
 
+static bool check_valid_off(lfs_off_t current_pos, lfs_soff_t off) {
+    /* Check if current_pos + off would be less than 0. We cannot use signed addition
+     * to check that because it would result in possible UB.
+     */
+    if (off == INT32_MIN) {
+        return false;
+    }
+
+    if (off < 0 && ((lfs_soff_t) current_pos) < -off) {
+        return false;
+    }
+
+    /* Unsigned overflow. */
+    if (off > 0 && (current_pos + ((lfs_off_t) off) < current_pos)) {
+        return false;
+    }
+
+    return true;
+}
+
 static lfs_soff_t lfs_file_seek_(lfs_t *lfs, lfs_file_t *file,
         lfs_soff_t off, int whence) {
     // find new pos
@@ -3668,17 +3688,16 @@ static lfs_soff_t lfs_file_seek_(lfs_t *lfs, lfs_file_t *file,
     if (whence == LFS_SEEK_SET) {
         npos = off;
     } else if (whence == LFS_SEEK_CUR) {
-        if ((lfs_soff_t)file->pos + off < 0) {
+        if (!check_valid_off(file->pos, off)) {
             return LFS_ERR_INVAL;
         } else {
             npos = file->pos + off;
         }
     } else if (whence == LFS_SEEK_END) {
-        lfs_soff_t res = lfs_file_size_(lfs, file) + off;
-        if (res < 0) {
+        if (!check_valid_off(lfs_file_size_(lfs, file), off)) {
             return LFS_ERR_INVAL;
         } else {
-            npos = res;
+            npos = lfs_file_size_(lfs, file) + off;
         }
     }