Re: [SECURITY PATCH 029/117] zstd: Initialize seq_t structure fully

[Darren Kenny]

Hi Paul,

On Thursday, 2021-03-18 at 09:50:00 +01, Paul Menzel wrote:
> Dear Darren, dear Daniel,
>
>
> Am 02.03.21 um 19:00 schrieb Daniel Kiper:
>> From: Darren Kenny <darren.kenny@oracle.com>
>> 
>> While many compilers will initialize this to zero, not all will,
>
> Which ones do not?

I have been working with C for a long time now, and there have been
compilers that don't initialize stack variables.

While many of the Linux compilers such as gcc and clang compilers often
do - depending on the optimization levels and building with debug or
not. (Debug code tends to be correctly zeroed out, but optimized doesn't
always)

I remember having to change a lot of GNOME C code being ported to
another platform, Solaris at the time, where that code always assumed
that it would be initialized to 0, and random things would happen
depending on what was on the stack.

The details of this are well documented and are a known security issue:

- CWE-457 (http://cwe.mitre.org/data/definitions/457.html)

>> so it is better to be sure that fields not being explicitly set are at known
>> values, and there is code that checks this fields value elsewhere in the
>> code.
>> 
>> Fixes: CID 292440
>
> What is the exact error? Is there a code flow, where one element does 
> not get set. (The commit message would be incorrect if this is not the 
> case.)

I can't honestly remember the details that Coverity had, but there was a
flow found by Coverity that did end up returning the value in seq
without touching all of the fields in the structure.

Looking through the code manually just now, I can see that seq.match is
never set during that code in ZSTD_decodeSequence(), so that is the most
likely cause of the error from Coverity.

All that it can do is report that it was not set before returning, it
cannot necessarily predict what will happen after that function returns
it, especially over time.

>
> Lastly, this is imported from upstream. I created an issue upstream [1].
>

Thanks, makes sense.

>> Signed-off-by: Darren Kenny <darren.kenny@oracle.com>
>> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>> ---
>>   grub-core/lib/zstd/zstd_decompress.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/grub-core/lib/zstd/zstd_decompress.c 
>> b/grub-core/lib/zstd/zstd_decompress.c
>> index 711b5b6d7..e4b5670c2 100644
>> --- a/grub-core/lib/zstd/zstd_decompress.c
>> +++ b/grub-core/lib/zstd/zstd_decompress.c
>> @@ -1325,7 +1325,7 @@ typedef enum { ZSTD_lo_isRegularOffset, 
>> ZSTD_lo_isLongOffset=1 } ZSTD_longOffset
>>   FORCE_INLINE_TEMPLATE seq_t
>>   ZSTD_decodeSequence(seqState_t* seqState, const ZSTD_longOffset_e 
>> longOffsets)
>>   {
>> -    seq_t seq;
>> +    seq_t seq = {0};
>>       U32 const llBits = 
>> seqState->stateLL.table[seqState->stateLL.state].nbAdditionalBits;
>>       U32 const mlBits = 
>> seqState->stateML.table[seqState->stateML.state].nbAdditionalBits;
>>       U32 const ofBits = 
>> seqState->stateOffb.table[seqState->stateOffb.state].nbAdditionalBits;
>> 
>
> I once read, that compilers cannot warn you, if you miss setting an 
> element if you initialize structures to 0 in the beginning.
>

That is true, since it has been initialized :)

But compilers won't always warn you either unless you ask them to, e.g.
gcc requires you to add the -Wuninitialized option, it isn't included in
-Wall (at lot aren't TBH, despite the name).

It is fine not to initialize while developing the code - if you want to
use that to catch such cases - but you should probably then also be
enabling every possible warning that a compiler may provide, maybe also
using some static code analysis (though compilers are improving a lot
here, e.g. in GCC 10) and most importantly fix them all before shipping
code :)

All to common is that people end up turning off the annoying warnings
rather than paying attention to them and resolving them.

Thanks,

Darren.

> Kind regards,
>
> Paul
>
>
> [1]: https://github.com/facebook/zstd/issues/2545