Received-SPF: pass (google.com: domain of linux-crypto+bounces-1065-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
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>
 <f27efd2e-ac65-4f6a-b1b5-c9fb0753d871@bytedance.com> <CAGsJ_4x31mT8TXt4c7ejJoDW1yJhyNqDmJmLZrf2LxMt7Zwg2A@mail.gmail.com>
 <5aff3bcf-ef36-45b3-8ac0-a4b19697419c@bytedance.com> <CAGsJ_4xuuaPnQzkkQVaRyZL6ZdwkiQ_B7_c2baNaCKVg_O7ZQA@mail.gmail.com>
 <e464c55e-d03a-4df9-abef-5ece182d7a2b@bytedance.com>
In-Reply-To: <e464c55e-d03a-4df9-abef-5ece182d7a2b@bytedance.com>
From: Barry Song <21cnbao@gmail.com>
Date: Wed, 27 Dec 2023 19:10:38 +0800
Message-ID: <CAGsJ_4xknGt5Q6vTKHdnqvXi7fKMORwXywurdN09G2uOUJzCSg@mail.gmail.com>
Subject: Re: [syzbot] [crypto?] general protection fault in
 scatterwalk_copychunks (5)
To: Chengming Zhou <zhouchengming@bytedance.com>
Cc: Nhat Pham <nphamcs@gmail.com>, Chris Li <chrisl@kernel.org>, 
	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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 27, 2023 at 5:16=E2=80=AFPM Chengming Zhou
<zhouchengming@bytedance.com> wrote:
>
> On 2023/12/27 15:01, Barry Song wrote:
> > On Wed, Dec 27, 2023 at 7:38=E2=80=AFPM Chengming Zhou
> > <zhouchengming@bytedance.com> wrote:
> >>
> >> On 2023/12/27 14:25, Barry Song wrote:
> >>> On Wed, Dec 27, 2023 at 4:51=E2=80=AFPM Chengming Zhou
> >>> <zhouchengming@bytedance.com> wrote:
> >>>>
> >>>> On 2023/12/27 08:23, Nhat Pham wrote:
> >>>>> On Tue, Dec 26, 2023 at 3:30=E2=80=AFPM Chris Li <chrisl@kernel.org=
> wrote:
> >>>>>>
> >>>>>> Again, sorry I was looking at the decompression side rather than t=
he
> >>>>>> 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 zs=
wap
> >>>>>> roll back to 2 page buffer until we have a safe way to do compress=
ion
> >>>>>> without overwriting the output buffers.
> >>>>>
> >>>>> Unfortunately, I think this is the way - at least until we rework t=
he
> >>>>> 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_sc=
ratch->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) does=
n'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 =3D *tfm_ctx;
> >>>>         void **ctx =3D acomp_request_ctx(req);
> >>>>         struct scomp_scratch *scratch;
> >>>> +       unsigned int dlen;
> >>>>         int ret;
> >>>>
> >>>>         if (!req->src || !req->slen || req->slen > SCOMP_SCRATCH_SIZ=
E)
> >>>> @@ -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 =3D SCOMP_SCRATCH_SIZE;
> >>>>
> >>>> +       dlen =3D req->dlen;
> >>>> +
> >>>>         scratch =3D 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 =3D -ENOMEM;
> >>>>                                 goto out;
> >>>>                         }
> >>>> +               } else if (req->dlen > dlen) {
> >>>> +                       ret =3D -ENOMEM;
> >>>> +                       goto out;
> >>>>                 }
> >>>
> >>> This can't fix the problem as crypto_scomp_compress() has written ove=
rflow data.
> >>
> >> No, crypto_scomp_compress() writes to its own scomp_scratch->dst memor=
y, then copy
> >> to our dstmem.
> >>
> >>>
> >>> BTW, in many cases, hardware-accelerators drivers/crypto can do compr=
ession and
> >>> decompression by off-loading CPU;
> >>> we won't have a chance to let hardware check the dst buffer size. so
> >>> giving the dst buffer
> >>> with enough length to the hardware's dma engine is the right way. I
> >>> mean, we shouldn't
> >>> change dst from 2pages to 1page.
> >>
> >> But how do we know 2 pages is enough for any compression algorithm?
> >>
> >
> > There is no guarrette 2 pages is enough.
> >
> > We can invent our own compression algorithm, in our algorithm, we can
> > add a lot of metadata, for example, longer than 4KB when the source dat=
a
> > is uncompress-able. then dst can be larger than 2 * PAGE_SIZE.  but thi=
s
> > is not the case :-) This kind of algorithm may never succeed.
> >
> > For those in-tree algorithms, we have a WORST_COMPR_FACTOR. in
> > ubifs, zram and zswap, we all use "2" as the worst comp factor.
>
> Thanks for your explanation! Maybe it's best for us to return to 2 pages
> if no other people's comments. And this really need more documentation :-=
)

I agree. we need some doc.

besides, i actually think we can skip zswap frontend if
over-compression is really
happening.

--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1318,7 +1318,7 @@ bool zswap_store(struct folio *folio)
        if (zpool_malloc_support_movable(zpool))
                gfp |=3D __GFP_HIGHMEM | __GFP_MOVABLE;
        ret =3D zpool_malloc(zpool, dlen, gfp, &handle);
-       if (ret =3D=3D -ENOSPC) {
+       if (ret =3D=3D -ENOSPC || dlen > PAGE_SIZE) {
                zswap_reject_compress_poor++;
                goto put_dstmem;
        }

since we are not saving but wasting space in this corner case?

> since there is no any comment or check in the acomp compress interface.
>
> /*
>  * @src:        Source Data
>  * @dst:        Destination data
>  * @slen:       Size of the input buffer
>  * @dlen:       Size of the output buffer and number of bytes produced
>  * @flags:      Internal flags
>  * @__ctx:      Start of private context data
>  */
> struct acomp_req {
>         struct crypto_async_request base;
>         struct scatterlist *src;
>         struct scatterlist *dst;
>         unsigned int slen;
>         unsigned int dlen;
>         u32 flags;
>         void *__ctx[] CRYPTO_MINALIGN_ATTR;
> };
>
> >
> > /*
> >  * Some compressors, like LZO, may end up with more data then the input=
 buffer.
> >  * So UBIFS always allocates larger output buffer, to be sure the compr=
essor
> >  * will not corrupt memory in case of worst case compression.
> >  */
> > #define WORST_COMPR_FACTOR 2
> >
> > #ifdef CONFIG_FS_ENCRYPTION
> > #define UBIFS_CIPHER_BLOCK_SIZE FSCRYPT_CONTENTS_ALIGNMENT
> > #else
> > #define UBIFS_CIPHER_BLOCK_SIZE 0
> > #endif
> >
> > /*
> >  * How much memory is needed for a buffer where we compress a data node=
.
> >  */
> > #define COMPRESSED_DATA_NODE_BUF_SZ \
> >         (UBIFS_DATA_NODE_SZ + UBIFS_BLOCK_SIZE * WORST_COMPR_FACTOR)
> >
> >
> > For years, we have never seen 2 pages that can be a problem. but 1
> > page is definitely
> > not enough, I remember I once saw many cases where accelerators' dmaeng=
ine
> > can write more than 1 page.
> >
> >> Thanks.
> >>
> >>>
> >>>>                 scatterwalk_map_and_copy(scratch->dst, req->dst, 0, =
req->dlen,
> >>>>                                          1);
> >>>
> >>>
>

Thanks
Barry