Received-SPF: pass (google.com: domain of linux-crypto+bounces-1051-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <f27efd2e-ac65-4f6a-b1b5-c9fb0753d871@bytedance.com>
Date: Wed, 27 Dec 2023 11:51:32 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [syzbot] [crypto?] general protection fault in
 scatterwalk_copychunks (5)
Content-Language: en-US
To: Nhat Pham <nphamcs@gmail.com>, Chris Li <chrisl@kernel.org>,
 21cnbao@gmail.com
Cc: syzbot <syzbot+3eff5e51bf1db122a16e@syzkaller.appspotmail.com>,
 akpm@linux-foundation.org, davem@davemloft.net, herbert@gondor.apana.org.au,
 linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
 syzkaller-bugs@googlegroups.com, yosryahmed@google.com
References: <0000000000000b05cd060d6b5511@google.com>
 <CAKEwX=OmWYivf7dg_izW8pn5s5q15+nx-vRMsV47T_qG=dep_Q@mail.gmail.com>
 <CAF8kJuPLEXEXG+4esR6MbRa3iirTrJ7-w3YCorB9iD=gnQ+G3A@mail.gmail.com>
 <CAKEwX=PaFmreqmNrisatSN1=k2kRiYgDksgDze-t=GBD=0iJDg@mail.gmail.com>
 <CAF8kJuPF5ACu8o1P7GqEQRb6p8QShyTVNuzrrY557g+SsddzWA@mail.gmail.com>
 <CAKEwX=NHdr9=hUBiZhnLZyRPsp=JwN3Vkwud2XEn3=pNurYGpQ@mail.gmail.com>
From: Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <CAKEwX=NHdr9=hUBiZhnLZyRPsp=JwN3Vkwud2XEn3=pNurYGpQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 2023/12/27 08:23, Nhat Pham wrote:
> On Tue, Dec 26, 2023 at 3:30 PM Chris Li <chrisl@kernel.org> wrote:
>>
>> Again, sorry I was looking at the decompression side rather than the
>> compression side. The compression side does not even offer a safe
>> version of the compression function.
>> That seems to be dangerous. It seems for now we should make the zswap
>> roll back to 2 page buffer until we have a safe way to do compression
>> without overwriting the output buffers.
> 
> Unfortunately, I think this is the way - at least until we rework the
> crypto/compression API (if that's even possible?).
> I still think the 2 page buffer is dumb, but it is what it is :(

Hi,

I think it's a bug in `scomp_acomp_comp_decomp()`, which doesn't use
the caller passed "src" and "dst" scatterlist. Instead, it uses its own
per-cpu "scomp_scratch", which have 128KB src and dst.

When compression done, it uses the output req->dlen to copy scomp_scratch->dst
to our dstmem, which has only one page now, so this problem happened.

I still don't know why the alg->compress(src, slen, dst, &dlen) doesn't
check the dlen? It seems an obvious bug, right?

As for this problem in `scomp_acomp_comp_decomp()`, this patch below
should fix it. I will set up a few tests to check later.

Thanks!

diff --git a/crypto/scompress.c b/crypto/scompress.c
index 442a82c9de7d..e654a120ae5a 100644
--- a/crypto/scompress.c
+++ b/crypto/scompress.c
@@ -117,6 +117,7 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
        struct crypto_scomp *scomp = *tfm_ctx;
        void **ctx = acomp_request_ctx(req);
        struct scomp_scratch *scratch;
+       unsigned int dlen;
        int ret;

        if (!req->src || !req->slen || req->slen > SCOMP_SCRATCH_SIZE)
@@ -128,6 +129,8 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
        if (!req->dlen || req->dlen > SCOMP_SCRATCH_SIZE)
                req->dlen = SCOMP_SCRATCH_SIZE;

+       dlen = req->dlen;
+
        scratch = raw_cpu_ptr(&scomp_scratch);
        spin_lock(&scratch->lock);

@@ -145,6 +148,9 @@ static int scomp_acomp_comp_decomp(struct acomp_req *req, int dir)
                                ret = -ENOMEM;
                                goto out;
                        }
+               } else if (req->dlen > dlen) {
+                       ret = -ENOMEM;
+                       goto out;
                }
                scatterwalk_map_and_copy(scratch->dst, req->dst, 0, req->dlen,
                                         1);