PING: [PATCH] Don't overflow when writing a key

[Richard Weinberger]

----- Ursprüngliche Mail -----
>> key_write() takes a ubifs_key as source and writes to a buffer.
>> It assumes that the passed ubifs_key is in simple key format (being UBIFS_SK_LEN
>> long)
>> and it assumes the buffer is UBIFS_MAX_KEY_LEN long.
>> So, the destination is *not* a union ubifs_key structure.
> 
> Ah. This is something I probably misunderstood. As the destination is
> casted to an ubifs_key as well, my expectation was that this function
> should not write outside the bounds of such an ubifs_key struct.
> 
>> We have three callers of key_write(), all in journal.c
>> 
>> 1. ubifs_jnl_write_data
>> key_write(c, key, &data->key);
>> 
>> data is struct ubifs_data_node where key is of size UBIFS_MAX_KEY_LEN.
>> 
>> 2. ubifs_jnl_update
>> 
>> Same with struct ubifs_dent_node
>> 
>> 3. ubifs_jnl_delete_xattr
>> 
>> Same again.
>> 
>> All three objects are slab allocated, did you check, is too few memory
>> allocated?
>> In other words, how did you calculate the size of target buffer?
> 
> You're right, the code is correct somehow, but was misleading me.
> 
>  The supplied buffer (ubifs_data_node.key) is actually
> UBIFS_MAX_KEY_LEN bytes long, but used as ubifs_key (which is smaller)
> and then the remaining space is zeroed out, thus there's an assumption
> about the buffer size.
> 
>  I actually found this due to a warning for key.h:439:
> 
> [mk all 2021-09-19 16:20:34]   arm-linux-gnueabihf-gcc
> -Wp,-MMD,fs/ubifs/.journal.o.d -nostdinc -isystem
> /var/lib/laminar/run/linux-arm-aspeed_g4_defconfig/8/toolchain/bin/../lib/gcc/arm-linux-gnueabihf/12.0.0/include
> -I./arch/arm/include -I./arch/arm/include/generated  -I./include
> -I./arch/arm/include/uapi -I./arch/arm/include/generated/uapi -I./include/uapi
> -I./include/generated/uapi -include ./include/linux/compiler-version.h -include
> ./include/linux/kconfig.h -include ./include/linux/compiler_types.h
> -D__KERNEL__ -mlittle-endian -fmacro-prefix-map=./= -Wall -Wundef
> -Werror=strict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common
> -fshort-wchar -fno-PIE -Werror=implicit-function-declaration
> -Werror=implicit-int -Werror=return-type -Wno-format-security -std=gnu89
> -fno-dwarf2-cfi-asm -fno-omit-frame-pointer -mapcs -mno-sched-prolog
> -fno-ipa-sra -mabi=aapcs-linux -mfpu=vfp -marm -Wa,-mno-warn-deprecated
> -D__LINUX_ARM_ARCH__=5 -march=armv5te -mtune=arm9tdmi -msoft-float -Uarm
> -fno-delete-null-pointer-checks -Wno-frame-address -Wno-format-truncation
> -Wno-format-overflow -Wno-address-of-packed-member -O2
> -fno-allow-store-data-races -Wframe-larger-than=1024 -fstack-protector-strong
> -Wimplicit-fallthrough=5 -Wno-main -Wno-unused-but-set-variable
> -Wno-unused-const-variable -fno-omit-frame-pointer -fno-optimize-sibling-calls
> -ftrivial-auto-var-init=zero
> -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang
> -fno-stack-clash-protection -g -gdwarf-4 -fno-var-tracking
> -femit-struct-debug-baseonly -pg -Wdeclaration-after-statement -Wvla
> -Wno-pointer-sign -Wno-stringop-truncation -Wno-zero-length-bounds
> -Wno-array-bounds -Wno-stringop-overflow -Wno-restrict -Wno-maybe-uninitialized
> -fno-strict-overflow -fno-stack-check -fconserve-stack -Werror=date-time
> -Werror=incompatible-pointer-types -Werror=designated-init
> -Wno-packed-not-aligned    -DKBUILD_MODFILE='"fs/ubifs/ubifs"'
> -DKBUILD_BASENAME='"journal"' -DKBUILD_MODNAME='"ubifs"'
> -D__KBUILD_MODNAME=kmod_ubifs -c -o fs/ubifs/journal.o fs/ubifs/journal.c
> [mk all 2021-09-19 16:20:35] In file included from ./include/linux/string.h:262,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/bitmap.h:10,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/cpumask.h:12,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/smp.h:13,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/lockdep.h:14,
> [mk all 2021-09-19 16:20:35]                  from
> ./include/linux/spinlock.h:63,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/wait.h:9,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/wait_bit.h:8,
> [mk all 2021-09-19 16:20:35]                  from ./include/linux/fs.h:6,
> [mk all 2021-09-19 16:20:35]                  from fs/ubifs/ubifs.h:16,
> [mk all 2021-09-19 16:20:35]                  from fs/ubifs/journal.c:49:
> [mk all 2021-09-19 16:20:35] In function 'memset',
> [mk all 2021-09-19 16:20:35]     inlined from 'key_write.part.0' at
> fs/ubifs/key.h:439:2:
> [mk all 2021-09-19 16:20:35] ./include/linux/fortify-string.h:172:17: error:
> call to '__write_overflow' declared with attribute error: detected write beyond
> size of object passed as 1st parameter
> [mk all 2021-09-19 16:20:35]   172 |                 __write_overflow();
> [mk all 2021-09-19 16:20:35]       |                 ^~~~~~~~~~~~~~~~~~
> [mk all 2021-09-19 16:20:35] make[2]: *** [scripts/Makefile.build:277:
> fs/ubifs/journal.o] Error 1
> [mk all 2021-09-19 16:20:35] make[1]: *** [scripts/Makefile.build:540: fs/ubifs]
> Error 2

Hmmm, fortify assumes that the buffer behind "to" is sizeof (union ubifs_key) just because
the function does:
union ubifs_key *t = to;
?

Even though memset() operates on "to" and not "t"...

> Maybe supply key_write() the complete target buffer length and
> memset() it first, then place the properly formatted key into it?

Well, that's the whole purpose of key_write(). It formats an in-memory key for
the disk format.
I fear this is just a matter of static analysis being not smart.

Thanks,
//richard