Received-SPF: pass (google.com: domain of linux-crypto+bounces-1058-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
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>
In-Reply-To: <5aff3bcf-ef36-45b3-8ac0-a4b19697419c@bytedance.com>
From: Barry Song <21cnbao@gmail.com>
Date: Wed, 27 Dec 2023 20:13:25 +1300
Message-ID: <CAGsJ_4yJEr21-HijCL8tSXuosF3WtrmB0pTLGPLvhQ6DD22QzA@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 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 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 zswa=
p
> >>>> roll back to 2 page buffer until we have a safe way to do compressio=
n
> >>>> 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 ow=
n
> >> per-cpu "scomp_scratch", which have 128KB src and dst.
> >>
> >> When compression done, it uses the output req->dlen to copy scomp_scra=
tch->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_re=
q *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_SIZE)
> >> @@ -128,6 +129,8 @@ static int scomp_acomp_comp_decomp(struct acomp_re=
q *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_re=
q *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 overf=
low data.
>
> No, crypto_scomp_compress() writes to its own scomp_scratch->dst memory, =
then copy
> to our dstmem.

Thanks, I got your point as you were using scomp. I used to depend on
acomp, so that
wasn't the case.

>
> >
> > BTW, in many cases, hardware-accelerators drivers/crypto can do compres=
sion 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?
>
> Thanks.
>
> >
> >>                 scatterwalk_map_and_copy(scratch->dst, req->dst, 0, re=
q->dlen,
> >>                                          1);

Thanks
 Barry