Bit Shift Overflow in lfs_frombe32 and lfs_fromle32 on MSP430 (16-bit Architecture)

[selimkeles]

Description

When compiling LittleFS on the MSP430 (a 16-bit architecture), the functions lfs_frombe32() and lfs_fromle32 causes incorrect results due to implicit integer promotion rules and bit shift behavior. Since int is 16-bit on MSP430, shifting beyond 16 bits leads to unintended overflows, resulting in incorrect byte-order conversions.

Affected Function

File: lfs_util.h
// Convert between 32-bit big-endian and native order
static inline uint32_t lfs_frombe32(uint32_t a) {
#if !defined(LFS_NO_INTRINSICS) && ( \
    (defined(  BYTE_ORDER  ) && defined(  ORDER_LITTLE_ENDIAN  ) &&   BYTE_ORDER   ==   ORDER_LITTLE_ENDIAN  ) || \
    (defined(__BYTE_ORDER  ) && defined(__ORDER_LITTLE_ENDIAN  ) && __BYTE_ORDER   == __ORDER_LITTLE_ENDIAN  ) || \
    (defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__))
    return __builtin_bswap32(a);
#elif (defined(  BYTE_ORDER  ) && defined(  ORDER_BIG_ENDIAN  ) &&   BYTE_ORDER   ==   ORDER_BIG_ENDIAN  ) || \
    (defined(__BYTE_ORDER  ) && defined(__ORDER_BIG_ENDIAN  ) && __BYTE_ORDER   == __ORDER_BIG_ENDIAN  ) || \
    (defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
    return a;
#else
    return (((uint8_t*)&a)[0] << 24) |     // Incorrect behavior on 16-bit platforms
           (((uint8_t*)&a)[1] << 16) |     // Incorrect behavior on 16-bit platforms
           (((uint8_t*)&a)[2] <<  8) |
           (((uint8_t*)&a)[3] <<  0);  
#endif
}

// Convert between 32-bit little-endian and native order
static inline uint32_t lfs_fromle32(uint32_t a) {
#if (defined(  BYTE_ORDER  ) && defined(  ORDER_LITTLE_ENDIAN  ) &&   BYTE_ORDER   ==   ORDER_LITTLE_ENDIAN  ) || \
    (defined(__BYTE_ORDER  ) && defined(__ORDER_LITTLE_ENDIAN  ) && __BYTE_ORDER   == __ORDER_LITTLE_ENDIAN  ) || \
    (defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
    return a;
#elif !defined(LFS_NO_INTRINSICS) && ( \
    (defined(  BYTE_ORDER  ) && defined(  ORDER_BIG_ENDIAN  ) &&   BYTE_ORDER   ==   ORDER_BIG_ENDIAN  ) || \
    (defined(__BYTE_ORDER  ) && defined(__ORDER_BIG_ENDIAN  ) && __BYTE_ORDER   == __ORDER_BIG_ENDIAN  ) || \
    (defined(__BYTE_ORDER__) && defined(__ORDER_BIG_ENDIAN__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__))
    return __builtin_bswap32(a);
#else
    return (((uint8_t*)&a)[0] <<  0) |
           (((uint8_t*)&a)[1] <<  8) |
           (((uint8_t*)&a)[2] << 16) |    // Incorrect behavior on 16-bit platforms
           (((uint8_t*)&a)[3] << 24);    // Incorrect behavior on 16-bit platforms
#endif
}

Issue Details

On MSP430, the bit shifts in the #else case result in an integer overflow due to implicit type promotion. Since int is 16-bit, shifting beyond 16 bits results in data loss or sign extension issues.

Proposed Fix

Explicitly cast each shifted value to uint32_t to ensure correct promotion and prevent overflow:

#else
    return ((uint32_t)((uint8_t*)&a)[0] << 24) |
           ((uint32_t)((uint8_t*)&a)[1] << 16) |
           ((uint32_t)((uint8_t*)&a)[2] <<  8) |
           ((uint32_t)((uint8_t*)&a)[3] <<  0);
#endif

#else
    return ((uint32_t)((uint8_t*)&a)[0] <<  0) |
           ((uint32_t)((uint8_t*)&a)[1] <<  8) |
           ((uint32_t)((uint8_t*)&a)[2] << 16) |
           ((uint32_t)((uint8_t*)&a)[3] << 24);
#endif

Impact

This issue affects all 16-bit architectures where int is not 32-bit.
The incorrect byte-order conversion may lead to corrupted metadata or failed mount operations in LittleFS.

Steps to Reproduce

Compile and run LittleFS on MSP430 or another 16-bit platform.
Observe incorrect behavior in functions relying on lfs_frombe32, such as mounting (lfs_mount).
Apply the proposed fix and verify that the issue no longer occurs.

Request

Please consider merging the proposed fix to ensure compatibility with 16-bit architectures. I am happy to contribute further testing or submit a PR if needed.

[geky]

Hi @selimkeles, sorry about the late response, thanks for this.

I'm surprised those shifts aren't broken even in 32-bit mode. The casts are good even if only documenting the implicit casts. Feel free to create a PR if you want attribution.

I am happy to contribute further testing

I would really like to add a 16-bit MCU to CI, but last time I briefly looked into it QEMU simply didn't support anything that's 16-bit (this may have changed). So I'm curious what the standard practice for emulating the MSP430 is.

Really the only blocker is finding an emulator. The test framework supports emulation with the --exec flag, so adding it to CI should just require adding another test step to test.yml:

littlefs/.github/workflows/test.yml

Lines 20 to 21 in 8ed63b2

	matrix:
	arch: [x86_64, thumb, mips, powerpc]

littlefs/.github/workflows/test.yml

Lines 46 to 57 in 8ed63b2

	# cross-compile with MIPS (32-bit, big-endian)
	- name: install-mips
	if: ${{matrix.arch == 'mips'}}
	run: |
	sudo apt-get install -qq \
	gcc-mips-linux-gnu \
	libc6-dev-mips-cross \
	qemu-user
	echo "CC=mips-linux-gnu-gcc --static" >> $GITHUB_ENV
	echo "EXEC=qemu-mips" >> $GITHUB_ENV
	mips-linux-gnu-gcc --version
	qemu-mips -version

That would also add code/stack measurements, which would be interesting to see. (I'm assuming GCC, things get a bit more complicated if not GCC.)

[selimkeles]

Hi @geky ,

Thanks for the response, I've just created PR. Also, I've just searched up that QEMU might be used for MSP430 emulator environment (Which roughly covers 16bit and maybe 8bit archs). But it needs little modifications. Found a fork targeting MSP430 archs which is 10 years old. I'd like to help in CI-CD side, but I should warn I never used QEMU before.

[geky]

Thanks! Will bring it in.

I think the hard part will be how to install in Ubuntu (GitHub Action job). For standard QEMU it's just apt-get install qemu-user, for this fork I assume it will require recompilation? I wonder how long that takes and if it will be a problem for GHA...

Once installed the actual emulation should be easy. Normally it's just qemu-my-arch ./program, but to add to littlefs's CI you probably want to just copy the above chunk of test.yml.