Message-ID: <202203301416.568595B87@keescook>
Date:   Wed, 30 Mar 2022 14:43:26 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     Nick Terrell <terrelln@...com>, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] lib: zstd: Fix Wstringop-overflow warning

On Wed, Mar 30, 2022 at 02:33:52PM -0500, Gustavo A. R. Silva wrote:
> Fix the following -Wstringop-overflow warning when building with GCC-11:
> 
> lib/zstd/decompress/huf_decompress.c: In function ‘HUF_readDTableX2_wksp’:
> lib/zstd/decompress/huf_decompress.c:700:5: warning: ‘HUF_fillDTableX2.constprop’ accessing 624 bytes in a region of size 52 [-Wstringop-overflow=]
>   700 |     HUF_fillDTableX2(dt, maxTableLog,
>       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   701 |                    wksp->sortedSymbol, sizeOfSort,
>       |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   702 |                    wksp->rankStart0, wksp->rankVal, maxW,
>       |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   703 |                    tableLog+1,
>       |                    ~~~~~~~~~~~
>   704 |                    wksp->calleeWksp, sizeof(wksp->calleeWksp) / sizeof(U32));
>       |                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/zstd/decompress/huf_decompress.c:700:5: note: referencing argument 6 of type ‘U32 (*)[13]’ {aka ‘unsigned int (*)[13]’}
> lib/zstd/decompress/huf_decompress.c:571:13: note: in a call to function ‘HUF_fillDTableX2.constprop’
>   571 | static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog,
>       |             ^~~~~~~~~~~~~~~~

Reviewing this changes would be easier if the reason for the warning
could be explained. i.e. why has GCC decided that the region is 52
bytes, and how did it calculate the 624 bytes?

rankVal_t is HUF_TABLELOG_MAX-many rankValCol_t, which itself is
HUF_TABLELOG_MAX + 1 many U32s. So, basically:

U32 array[HUF_TABLELOG_MAX + 1][HUF_TABLELOG_MAX]

sizeof(rankValCol_t) == 52
sizeof(rankVal_t) == 624

> 
> by using pointer notation instead of array notation.
> 
> This helps with the ongoing efforts to globally enable
> -Wstringop-overflow.
> 
> Link: https://github.com/KSPP/linux/issues/181
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> ---
>  lib/zstd/decompress/huf_decompress.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c
> index 5105e59ac04a..0ea34621253a 100644
> --- a/lib/zstd/decompress/huf_decompress.c
> +++ b/lib/zstd/decompress/huf_decompress.c
> @@ -570,7 +570,7 @@ static void HUF_fillDTableX2Level2(HUF_DEltX2* DTable, U32 sizeLog, const U32 co
>  
>  static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog,
>                             const sortedSymbol_t* sortedList, const U32 sortedListSize,
> -                           const U32* rankStart, rankVal_t rankValOrigin, const U32 maxWeight,
> +                           const U32* rankStart, const U32* rankValOrigin, const U32 maxWeight,
>                             const U32 nbBitsBaseline, U32* wksp, size_t wkspSize)

This really feels like we're papering over the warning. This removes the
type information and makes it a U32 * instead, and then later makes a
cast?

Can this be fixed in a way that retains the type information?

On the other hand, all the arguments are also U32 *.

I see stuff like:

    ZSTD_memcpy(rankVal, rankValOrigin, sizeof(U32) * (HUF_TABLELOG_MAX + 1));

That looks like it's ignoring type information as well. i.e. why isn't
this sizeof(rankValOrigin)? (The length above is 52 bytes.)

I'm especially curious here since rankValOrigin is rankVal_t, which is
624 bytes, not 52 bytes (i.e. the above is copying a single rankValCol_t
from rankValOrigin. I'd expect this to be:

    ZSTD_memcpy(rankVal, &rankValOrigin[0], sizeof(rankValOrigin[0]));


>  {
>      U32* rankVal = wksp;
> @@ -598,7 +598,7 @@ static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog,
>              if (minWeight < 1) minWeight = 1;
>              sortedRank = rankStart[minWeight];
>              HUF_fillDTableX2Level2(DTable+start, targetLog-nbBits, nbBits,
> -                           rankValOrigin[nbBits], minWeight,
> +                           rankValOrigin + nbBits, minWeight,

And here I'd expect to pass	&rankValOrigin[nbBits]
since HUF_fillDTableX2Level2 is doing another rankValCol_t-sized copy:

    ZSTD_memcpy(rankVal, rankValOrigin, sizeof(U32) * (HUF_TABLELOG_MAX + 1));


>                             sortedList+sortedRank, sortedListSize-sortedRank,
>                             nbBitsBaseline, symbol, wksp, wkspSize);
>          } else {
> @@ -699,7 +699,7 @@ size_t HUF_readDTableX2_wksp(HUF_DTable* DTable,
>  
>      HUF_fillDTableX2(dt, maxTableLog,
>                     wksp->sortedSymbol, sizeOfSort,
> -                   wksp->rankStart0, wksp->rankVal, maxW,
> +                   wksp->rankStart0, (U32 *)wksp->rankVal, maxW,

It's possible the problem is with this structure:

typedef struct {
    rankValCol_t rankVal[HUF_TABLELOG_MAX];
    U32 rankStats[HUF_TABLELOG_MAX + 1];
    U32 rankStart0[HUF_TABLELOG_MAX + 2];
    sortedSymbol_t sortedSymbol[HUF_SYMBOLVALUE_MAX + 1];
    BYTE weightList[HUF_SYMBOLVALUE_MAX + 1];
    U32 calleeWksp[HUF_READ_STATS_WORKSPACE_SIZE_U32];
} HUF_ReadDTableX2_Workspace;

it's not using the rankVal_t type for rankVal for some reason?

i.e. what's passed to HUF_fillDTableX2 is a rankValCol_t (52 bytes), but then it
gets passed against later as rankVal_t (624 bytes).

Does changing the type definition above solve this more cleanly?

-Kees

>                     tableLog+1,
>                     wksp->calleeWksp, sizeof(wksp->calleeWksp) / sizeof(U32));
>  
> -- 
> 2.27.0
> 

-- 
Kees Cook

Powered by blists - more mailing lists