| Message ID | 20211001192836.n6eek7rno7ewvmu7@lug-owl.de |
|---|---|
| State | Rejected |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | Don't overflow when writing a key | expand |
Hi, On Fri, 2021-10-01 21:28:36 +0200, Jan-Benedict Glaw <jbglaw@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); > } > > /** I wanted to give this little patch a ping since there wasn't a reply until now and I think it might fix an overflow. Please keep me Cc'ed since I'm not on this list! Thanks, Jan-Benedict --
----- Ursprüngliche Mail ----- > Von: "Jan-Benedict Glaw" <jbglaw@lug-owl.de> > An: "linux-mtd" <linux-mtd@lists.infradead.org> > CC: "Artem Bityutskiy" <artem.bityutskiy@linux.intel.com>, "richard" <richard@nod.at> > Gesendet: Dienstag, 5. Oktober 2021 16:49:36 > Betreff: PING: [PATCH] Don't overflow when writing a key > Hi, > > On Fri, 2021-10-01 21:28:36 +0200, Jan-Benedict Glaw <jbglaw@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); >> } >> >> /** > > I wanted to give this little patch a ping since there wasn't a reply In MTD's patchwork it is marked as "Under review", so it is not lost. > until now and I think it might fix an overflow. Your fix looks legit but since I'm traveling I need to give it a deeper thought when I'm back home. I'm a little puzzled why nobody noticed the stack corruption so far, key_write() has been doing since ever. Thanks, //richard
----- Ursprüngliche Mail ----- > Von: "richard" <richard@nod.at> > An: "Jan-Benedict Glaw" <jbglaw@lug-owl.de> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Artem Bityutskiy" <artem.bityutskiy@linux.intel.com> > Gesendet: Donnerstag, 7. Oktober 2021 08:59:03 > Betreff: Re: PING: [PATCH] Don't overflow when writing a key > ----- Ursprüngliche Mail ----- >> Von: "Jan-Benedict Glaw" <jbglaw@lug-owl.de> >> An: "linux-mtd" <linux-mtd@lists.infradead.org> >> CC: "Artem Bityutskiy" <artem.bityutskiy@linux.intel.com>, "richard" >> <richard@nod.at> >> Gesendet: Dienstag, 5. Oktober 2021 16:49:36 >> Betreff: PING: [PATCH] Don't overflow when writing a key > >> Hi, >> >> On Fri, 2021-10-01 21:28:36 +0200, Jan-Benedict Glaw <jbglaw@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); >>> } >>> >>> /** >> >> I wanted to give this little patch a ping since there wasn't a reply > > In MTD's patchwork it is marked as "Under review", so it is not lost. > >> until now and I think it might fix an overflow. > > Your fix looks legit but since I'm traveling I need to give it a deeper > thought when I'm back home. > I'm a little puzzled why nobody noticed the stack corruption so far, > key_write() has been doing since ever. Typing that mail triggered already a deeper thought. :-D 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. 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? Thanks, //richard
Hi Richard, On Thu, 2021-10-07 09:20:05 +0200, Richard Weinberger <richard@nod.at> wrote: > > > On Fri, 2021-10-01 21:28:36 +0200, Jan-Benedict Glaw <jbglaw@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 --
----- 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
On Thu, 2021-10-07 16:19:06 +0200, Richard Weinberger <richard@nod.at> wrote: > 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. Guess so... Maybe it'll get better. At least, that's been with a very modern GCC, so I guess this'll come up again in one way or another. (Either as a GCC bug, or by not simply having a key[] array were the data is "magically" written to, but some union with that array and probably a struct like struct ubifs_key (where, for practical purposes, it's __le32 j32 already is the on-disk representation.) So ... The code does _not_ overflow, that's good first of all. It's just a bit confusing how the key bytes are written to the buffer. At least I was easily distracted by the two different sizes. Thanks, Jan-Benedict --
----- Ursprüngliche Mail ----- > Von: "Jan-Benedict Glaw" <jbglaw@lug-owl.de> > An: "richard" <richard@nod.at> > > So ... The code does _not_ overflow, that's good first of all. It's > just a bit confusing how the key bytes are written to the buffer. At > least I was easily distracted by the two different sizes. If you find a better way to write the function, I'm all open for a patch. But please make sure the generated code is sane. Thanks, //richard
On Thu, 2021-10-07 17:20:47 +0200, Richard Weinberger <richard@nod.at> wrote: > > So ... The code does _not_ overflow, that's good first of all. It's > > just a bit confusing how the key bytes are written to the buffer. At > > least I was easily distracted by the two different sizes. > > If you find a better way to write the function, I'm all open for a patch. > But please make sure the generated code is sane. That wouldn't be terribly complicated, but I have no way to actually *test* that. It's just an issue I found while doing mass-builds for all of Linux's defconfigs. If there's anybody around helping with testing, I'd happily work on that. Thanks, Jan-Benedict --
----- Ursprüngliche Mail ----- > Von: "Jan-Benedict Glaw" <jbglaw@lug-owl.de> > An: "richard" <richard@nod.at> > CC: "linux-mtd" <linux-mtd@lists.infradead.org>, "Artem Bityutskiy" <artem.bityutskiy@linux.intel.com> > Gesendet: Donnerstag, 7. Oktober 2021 19:45:10 > Betreff: Re: PING: [PATCH] Don't overflow when writing a key > On Thu, 2021-10-07 17:20:47 +0200, Richard Weinberger <richard@nod.at> wrote: >> > So ... The code does _not_ overflow, that's good first of all. It's >> > just a bit confusing how the key bytes are written to the buffer. At >> > least I was easily distracted by the two different sizes. >> >> If you find a better way to write the function, I'm all open for a patch. >> But please make sure the generated code is sane. > > That wouldn't be terribly complicated, but I have no way to actually > *test* that. It's just an issue I found while doing mass-builds for You can test it. $ modprobe nandsim $ modprobe ubi $ modprobe ubifs $ ubiattach -m0 $ ubimkvol -m -N test /dev/ubi0 $ mount -t ubifs /dev/ubi0_0 /mnt Now you have a shiny ubifs on /mnt. :-) Thanks, //richard
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); } /**