Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <202308281119.10472FC7@keescook> <CAHk-=wi_WxZ2dEsQR0-wDtYAh4sxVEQkU7HK5JSboVv7v7NwcQ@mail.gmail.com>
 <B085ADB4-4B8C-4998-BB33-DA67C45483E9@kernel.org> <CAHk-=wjRD_LnCbwSRM20Fg54xhrFBLwgO=X23bdconx3wKokxg@mail.gmail.com>
 <202308282035.5AFD431B@keescook> <CAHk-=whbqsYC4nDc7HpWEYo7EnA603T35jSP4om6HMWpVZSc_w@mail.gmail.com>
In-Reply-To: <CAHk-=whbqsYC4nDc7HpWEYo7EnA603T35jSP4om6HMWpVZSc_w@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Tue, 29 Aug 2023 19:29:31 +0200
Message-ID: <CAMj1kXGJCBj2JQFkUgd26H-abagcpO+7z_--6HV42VeaqCsEnQ@mail.gmail.com>
Subject: Re: [GIT PULL] pstore updates for v6.6-rc1
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Kees Cook <keescook@chromium.org>, Kees Cook <kees@kernel.org>,
        linux-kernel@vger.kernel.org, Enlin Mu <enlin.mu@unisoc.com>,
        Eric Biggers <ebiggers@google.com>,
        "Guilherme G. Piccoli" <gpiccoli@igalia.com>,
        "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Yunlong Xing <yunlong.xing@unisoc.com>,
        Yuxiao Zhang <yuxiaozhang@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 29 Aug 2023 at 19:13, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, 28 Aug 2023 at 20:44, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Aug 28, 2023 at 06:44:02PM -0700, Linus Torvalds wrote:
> > > The only thing that is new is the kernel pstore implementation. Why
> > > was this not a problem before? The warning existed back then too, but
> > > I never actually got it.
> >
> > Right -- if the compression method from before was different, it'll fail
> > now. (i.e. we removed everything but zlib.)
>
> I don't think that was the case.
>
> Looking back in my logs, I see lines like this:
>
>   Aug 07 16:59:29 ryzen kernel: pstore: Using crash dump compression: deflate
>
> and while it appears F37 used to support other formats, it does have
>
>   CONFIG_PSTORE_DEFLATE_COMPRESS_DEFAULT=y
>
> so it should all be zlib-compatible from what I can tell.
>

-5 == Z_BUF_ERROR which is only returned by zlib_inflate() in one
particular case (according the the kerneldoc):

    In this implementation, the flush parameter of inflate() only affects the
   return code (per zlib.h).  inflate() always writes as much as possible to
   strm->next_out, given the space available and the provided input--the effect
   documented in zlib.h of Z_SYNC_FLUSH.  Furthermore, inflate() always defers
   the allocation of and copying into a sliding window until necessary, which
   provides the effect documented in zlib.h for Z_FINISH when the entire input
   stream available.  So the only thing the flush parameter actually does is:
   when flush is set to Z_FINISH, inflate() cannot return Z_OK.  Instead it
   will return Z_BUF_ERROR if it has not reached the end of the stream.

and the crypto compress wrapper for inflate does

        ret = zlib_inflate(stream, Z_SYNC_FLUSH);
        /*
         * Work around a bug in zlib, which sometimes wants to taste an extra
         * byte when being used in the (undocumented) raw deflate mode.
         * (From USAGI).
         */
        if (ret == Z_OK && !stream->avail_in && stream->avail_out) {
                u8 zerostuff = 0;
                stream->next_in = &zerostuff;
                stream->avail_in = 1;
                ret = zlib_inflate(stream, Z_FINISH);
        }

IOW, it does not use Z_FINISH but Z_SYNC_FLUSH for the primary
invocation, and only stuffs in one additional NUL byte if it returns
Z_OK instead of Z_STREAM_END.

This is an oversight on my part. The diff below plugs this into the pstore code

--- a/fs/pstore/platform.c
+++ b/fs/pstore/platform.c
@@ -593,7 +593,13 @@ static void decompress_record(struct pstore_record *record,
        zstream->next_out       = workspace;
        zstream->avail_out      = psinfo->bufsize;

-       ret = zlib_inflate(zstream, Z_FINISH);
+       ret = zlib_inflate(zstream, Z_SYNC_FLUSH);
+       if (ret == Z_OK && !zstream->avail_in && zstream->avail_out) {
+               u8 zerostuff = 0;
+               zstream->next_in = &zerostuff;
+               zstream->avail_in = 1;
+               ret = zlib_inflate(zstream, Z_FINISH);
+       }
        if (ret != Z_STREAM_END) {
                pr_err("zlib_inflate() failed, ret = %d!\n", ret);
                kvfree(workspace);