[PATCH] Don't overflow when writing a key

[PATCH] Don't overflow when writing a key

[Jan-Benedict Glaw]

[-- Attachment #1.1: Type: text/plain, Size: 4583 bytes --]

Hi!

I'm mass-building the GNU Toolchain, Linux, NetBSD and others and got
a warning with recent GCC versions when building Linux's
ARM-based aspeed_g4_defconfig:

[mk all 2021-10-01 14:10:14]   arm-linux-gnueabihf-gcc -Wp,-MMD,fs/ubifs/.journal.o.d -nostdinc -isystem /var/lib/laminar/run/linux-arm-aspeed_g4_defconfig/9/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-10-01 14:10:15] In file included from ./include/linux/string.h:262,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/bitmap.h:10,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/cpumask.h:12,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/smp.h:13,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/lockdep.h:14,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/spinlock.h:63,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/wait.h:9,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/wait_bit.h:8,
[mk all 2021-10-01 14:10:15]                  from ./include/linux/fs.h:6,
[mk all 2021-10-01 14:10:15]                  from fs/ubifs/ubifs.h:16,
[mk all 2021-10-01 14:10:15]                  from fs/ubifs/journal.c:49:
[mk all 2021-10-01 14:10:15] In function 'memset',
[mk all 2021-10-01 14:10:15]     inlined from 'key_write.part.0' at fs/ubifs/key.h:439:2:
[mk all 2021-10-01 14:10:15] ./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-10-01 14:10:15]   172 |                 __write_overflow();
[mk all 2021-10-01 14:10:15]       |                 ^~~~~~~~~~~~~~~~~~
[mk all 2021-10-01 14:10:15] make[2]: *** [scripts/Makefile.build:277: fs/ubifs/journal.o] Error 1
[mk all 2021-10-01 14:10:15] make[1]: *** [scripts/Makefile.build:540: fs/ubifs] Error 2
[mk all 2021-10-01 14:10:15] make: *** [Makefile:1868: fs] Error 2


This seems to be correct as the struct is declared using
UBIFS_SK_LEN (= 8), but used to use UBIFS_MAX_KEY_LEN (= 16), which
would overflow by 8 bytes.

Here's a suggested patch that is NOT tested as I don't have the UBIFS
in use (knowingly) anywhere:

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);
 }
 
 /**



Please keep me in Cc: as I'm not subscribed.

Thanks,
  Jan-Benedict

-- 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Jan-Benedict Glaw]

[-- Attachment #1.1: Type: text/plain, Size: 715 bytes --]

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

-- 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Richard Weinberger]

----- 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Richard Weinberger]

----- 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Jan-Benedict Glaw]

[-- Attachment #1.1: Type: text/plain, Size: 5754 bytes --]

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

-- 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Jan-Benedict Glaw]

[-- Attachment #1.1: Type: text/plain, Size: 1254 bytes --]

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

-- 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Richard Weinberger]

----- 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Jan-Benedict Glaw]

[-- Attachment #1.1: Type: text/plain, Size: 703 bytes --]

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

-- 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Richard Weinberger]

----- 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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/