linux-kernel - Re: [GIT PULL] Zstd fixes for v6.7

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]

Message-ID: <CAHk-=wgSBZG6ZaYe0pFm7iJL9Yab64zGdOVkLg2-FfhsXTtx+g@mail.gmail.com>
Date:   Tue, 14 Nov 2023 20:31:57 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Nick Terrell <terrelln@...a.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Nick Terrell <nickrterrell@...il.com>
Subject: Re: [GIT PULL] Zstd fixes for v6.7

On Tue, 14 Nov 2023 at 17:17, Nick Terrell <terrelln@...a.com> wrote:
>
> Only a single line change to fix a benign UBSAN warning that has been
> baking in linux-next for a month. I just missed the merge window, but I
> think it is worthwhile to include this fix in the v6.7 kernel. If you
> would like me to wait for v6.8 please let me know.

Hmm. You claim it's been in linux-next for a month, but why the hell
was it then rebased *minutes* before you sent the pull request?

That's really not ok. Rebasing literally removes the test coverage you
had. What possible reason was there for rebasing? And why didn't you
mention it?

So stop doing these dodgy things.

I have pulled this, but despite this being a "trivial" one-liner, I
think there is a bug in there somewhere.

In particular, we *used* to have

  typedef struct {
       short ncount[FSE_MAX_SYMBOL_VALUE + 1];
       FSE_DTable dtable[1]; /* Dynamically sized */
  } FSE_DecompressWksp;

and in FSE_decompress_wksp_body() we have

    FSE_DecompressWksp* const wksp = (FSE_DecompressWksp*)workSpace;
    ...
    if (wkspSize < sizeof(*wksp)) return ERROR(GENERIC);
    ...
    wkspSize -= sizeof(*wksp) + FSE_DTABLE_SIZE(tableLog);

and note that "sizeof(*wksp)".

Because it has *changed* with that one-liner fix, since now we have an
unsized array there:

  typedef struct {
       short ncount[FSE_MAX_SYMBOL_VALUE + 1];
       FSE_DTable dtable[]; /* Dynamically sized */
  } FSE_DecompressWksp;

so while the logic actually looks better to me with the change (no
more off-by-one errors), the fact that it used to work with what looks
like an off-by-one error in the sizeof() calculation just makes me go
"Hmm".

In particular, that

    wkspSize -= sizeof(*wksp) + FSE_DTABLE_SIZE(tableLog);

seems to have removed too much from the wkspSize variable, but it
still ended up not triggering any limit checks. Hmm?

End result: this may be a one-liner change, but honestly, I think it
was done HORRIBLY BADLY. That one-liner has serious implications and
just a trivial check of mine seems to say this code is or was seriosly
buggy exactlky when it comes to that one-liner.

And no, rebasing minutes before sending a pull request is not ok.

                   Linus