linux-kernel - Re: [syzbot] [crypto?] general protection fault in scatterwalk

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]

Date: Thu, 28 Dec 2023 11:18:14 -0800
From: Nhat Pham <nphamcs@...il.com>
To: Barry Song <21cnbao@...il.com>
Cc: Chengming Zhou <zhouchengming@...edance.com>, Chris Li <chrisl@...nel.org>, 
	syzbot <syzbot+3eff5e51bf1db122a16e@...kaller.appspotmail.com>, 
	akpm@...ux-foundation.org, davem@...emloft.net, herbert@...dor.apana.org.au, 
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org, 
	syzkaller-bugs@...glegroups.com, yosryahmed@...gle.com
Subject: Re: [syzbot] [crypto?] general protection fault in
 scatterwalk_copychunks (5)

On Wed, Dec 27, 2023 at 5:47 PM Barry Song <21cnbao@...il.com> wrote:
>
> On Thu, Dec 28, 2023 at 9:43 AM Barry Song <21cnbao@...il.com> wrote:
> >
> > On Thu, Dec 28, 2023 at 7:26 AM Nhat Pham <nphamcs@...il.com> wrote:
> > >
> > > On Wed, Dec 27, 2023 at 3:10 AM Barry Song <21cnbao@...il.com> wrote:
> > > >
> > > > On Wed, Dec 27, 2023 at 5:16 PM Chengming Zhou
> > > > <zhouchengming@...edance.com> wrote:
> > > > >
> > > > > 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 :-)
> > >
> > > Fine by me. Hmm we're basically wasting one extra page per CPU (since
> > > these buffers are per-CPU), correct? That's not ideal, but not *too*
> > > bad for now I suppose...
> > >
> > > >
> > > > I agree. we need some doc.
> > > >
> > > > besides, i actually think we can skip zswap frontend if
> > > > over-compression is really
> > > > happening.
> > >
> > > IIUC, zsmalloc already checked that - and most people are (or should
> > > be) using zsmalloc for zswap anyway. I wouldn't be opposed to adding
> > > an added layer of protection on the zswap side, but not super high
> > > priority I'd say.
> >
> > Thanks for this info. I guess you mean the below ?
> > unsigned long zs_malloc(struct zs_pool *pool, size_t size, gfp_t gfp)
> > {
> >         ...
> >
> >         if (unlikely(!size || size > ZS_MAX_ALLOC_SIZE))
> >                 return (unsigned long)ERR_PTR(-EINVAL);
>
> BTW, do you think zsmalloc is worth a patch to change the ret from
> EINVAL to ENOSPC?
> This seems more sensible and matches the behaviour of zswap, and zbud, z3fold.
>
>         ret = zpool_malloc(zpool, dlen, gfp, &handle);
>         if (ret == -ENOSPC) {
>                 zswap_reject_compress_poor++;
>                 goto put_dstmem;
>         }
>         if (ret) {
>                 zswap_reject_alloc_fail++;
>                 goto put_dstmem;
>         }
>         buf = z
>

I haven't looked at the code surrounding it too closely, but IMHO this
would be nice. Poor compressibility of the workload's memory is
something we should monitor more carefully. This has come up a couple
times in discussion:

https://lore.kernel.org/linux-mm/CAJD7tkZXS-UJVAFfvxJ0nNgTzWBiqepPYA4hEozi01_qktkitg@mail.gmail.com/
https://lore.kernel.org/all/20231024234509.2680539-1-nphamcs@gmail.com/T/#u

> >
> > }
> >
> > i find zbud also has similar code:
> > static int zbud_alloc(struct zbud_pool *pool, size_t size, gfp_t gfp,
> >                         unsigned long *handle)
> > {
> >         int chunks, i, freechunks;
> >         struct zbud_header *zhdr = NULL;
> >         enum buddy bud;
> >         struct page *page;
> >
> >         if (!size || (gfp & __GFP_HIGHMEM))
> >                 return -EINVAL;
> >         if (size > PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE)
> >                 return -ENOSPC;
> >
> > and z3fold,
> >
> > static int z3fold_alloc(struct z3fold_pool *pool, size_t size, gfp_t gfp,
> >                         unsigned long *handle)
> > {
> >         int chunks = size_to_chunks(size);
> >         struct z3fold_header *zhdr = NULL;
> >         struct page *page = NULL;
> >         enum buddy bud;
> >         bool can_sleep = gfpflags_allow_blocking(gfp);
> >
> >         if (!size || (gfp & __GFP_HIGHMEM))
> >                 return -EINVAL;
> >
> >         if (size > PAGE_SIZE)
> >                 return -ENOSPC;
> >
> >
> > Thus, I agree that another layer to check size in zswap isn't necessary now.
>
> Thanks
> Barry