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

Thu Oct 7 06:54:01 PDT 2021

Hi Richard,

On Thu, 2021-10-07 09:20:05 +0200, Richard Weinberger <richard at nod.at> wrote:
> > > On Fri, 2021-10-01 21:28:36 +0200, Jan-Benedict Glaw <jbglaw at lug-owl.de> wrote:
> > > > diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
> > > > index 8142d9d6fe5d..40edcca7ba62 100644
> > > > --- a/fs/ubifs/key.h
> > > > +++ b/fs/ubifs/key.h
> > > > @@ -436,7 +436,7 @@ static inline void key_write(const struct ubifs_info *c,
> > > >  
> > > >  	t->j32[0] = cpu_to_le32(from->u32[0]);
> > > >  	t->j32[1] = cpu_to_le32(from->u32[1]);
> > > > -	memset(to + 8, 0, UBIFS_MAX_KEY_LEN - 8);
> > > > +	memset(to + 8, 0, UBIFS_SK_LEN - 8);
> > > >  }
> > > >  
> > > >  /**

> 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

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

MfG, JBG

-- 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mtd/attachments/20211007/af659732/attachment.sig>