linux-hardening - Re: [PATCH v2] lib: zstd: Fix -Wstringop-overflow warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Jan 2023 23:10:08 +0000
From:   Nick Terrell <terrelln@...a.com>
To:     Kees Cook <keescook@...omium.org>
CC:     "Gustavo A . R . Silva" <gustavoars@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH v2] lib: zstd: Fix -Wstringop-overflow warning



> On Jan 4, 2023, at 1:20 PM, Kees Cook <keescook@...omium.org> wrote:
> 
> !-------------------------------------------------------------------|
>  This Message Is From an External Sender
> 
> |-------------------------------------------------------------------!
> 
> 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,
>      |             ^~~~~~~~~~~~~~~~
> 
> by using pointer notation instead of array notation.
> 
> This is one of the last remaining warnings to be fixed before globally
> enabling -Wstringop-overflow.

The patch looks correct to me, thanks for reviving it. But, I was attempting to reproduce the issue,
so I could better understand what's going on, and I wasn't able to reproduce it myself.

To attempt to reproduce, I applied this patch

---
diff --git a/lib/zstd/Makefile b/lib/zstd/Makefile
index 20f08c644b71..190d3d5ab4be 100644
--- a/lib/zstd/Makefile
+++ b/lib/zstd/Makefile
@@ -12,6 +12,8 @@ obj-$(CONFIG_ZSTD_COMPRESS) += zstd_compress.o
 obj-$(CONFIG_ZSTD_DECOMPRESS) += zstd_decompress.o
 obj-$(CONFIG_ZSTD_COMMON) += zstd_common.o
 
+ccflags-y := -Wstringop-overflow=4 -Werror
+
 zstd_compress-y := \
                zstd_compress_module.o \
                compress/fse_compress.o \
---

Then compiled on x86-64 with gcc 12.2.0 on tag v6.2-rc3. I saw no errors.
I also tried with just `-Wstringop-overflow`, and on upstream zstd. I tried to
make a minimal reproducer on godbolt, so I could see if it was the gcc version,
but wasn't able to make it fail with any of them https://gcc.godbolt.org/z/Exzq9arMr.

Could you please tell me how to reproduce this warning?

Best,
Nick Terrell

> Co-developed-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> Cc: Nick Terrell <terrelln@...com>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> v2: use "rankValCol_t *" instead of U32
> v1: https://lore.kernel.org/lkml/20220330193352.GA119296@embeddedor/
> ---
> lib/zstd/decompress/huf_decompress.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/zstd/decompress/huf_decompress.c b/lib/zstd/decompress/huf_decompress.c
> index 89b269a641c7..60958afebc41 100644
> --- a/lib/zstd/decompress/huf_decompress.c
> +++ b/lib/zstd/decompress/huf_decompress.c
> @@ -985,7 +985,7 @@ static void HUF_fillDTableX2Level2(HUF_DEltX2* DTable, U32 targetLog, const U32
> 
> static void HUF_fillDTableX2(HUF_DEltX2* DTable, const U32 targetLog,
>                            const sortedSymbol_t* sortedList,
> -                           const U32* rankStart, rankVal_t rankValOrigin, const U32 maxWeight,
> +                           const U32* rankStart, rankValCol_t *rankValOrigin, const U32 maxWeight,
>                            const U32 nbBitsBaseline)
> {
>     U32* const rankVal = rankValOrigin[0];
> -- 
> 2.34.1
>