Re: [PATCH] zsmalloc: zero-initialize zspage memory to prevent KMSAN uninit reads

[Sergey Senozhatsky]

Adding Yosry and Herbert,

On (26/05/12 14:47), Andrew Morton wrote:
> > Pages allocated via alloc_zpdesc() use alloc_pages_node() without
> > __GFP_ZERO, leaving physical memory uninitialized. When a compressed
> > object spans two physical pages in a zspage, zs_obj_read_sg_begin()
> > sets up a scatterlist pointing directly at the raw second page. If the
> > second page was freshly allocated and never written beyond the object
> > boundary, KMSAN detects reads of uninitialized memory downstream in
> > the decompressor (e.g. sw842_decompress reading the CRC trailer).

I don't get this.  How can sw842_decompress() read more bytes than
it's told to decompress.  We first compress and store the object,
before we load and decompress, reading past the known compressed
object size (which we pass to decompress function) should not happen.
Yosry, Herbert, any ideas?

> > Fix this by passing __GFP_ZERO to alloc_zpdesc() in alloc_zspage() so
> > all pages backing a zspage are zero-initialized at allocation time.
> > 
> > Reported-by: syzbot+8f77ff6144a73f0cf71b@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=8f77ff6144a73f0cf71b
> > Signed-off-by: Kartik Nair <contact.kartikn@xxxxxxxxx>
> 
> Thanks.
> 
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -951,7 +951,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
> >  	for (i = 0; i < class->pages_per_zspage; i++) {
> >  		struct zpdesc *zpdesc;
> >  
> > -		zpdesc = alloc_zpdesc(gfp, nid);
> > +		zpdesc = alloc_zpdesc(gfp | __GFP_ZERO, nid);
> >  		if (!zpdesc) {
> >  			while (--i >= 0) {
> >  				zpdesc_dec_zone_page_state(zpdescs[i]);
> 
> Decompressing uninitialized memory sounds rather bad, so I'll add a
> cc:stable to this.
[..]
> I think the Fixes: target is 56e5a103a721 ("zsmalloc: prefer the the
> original page's node for compressed data").  Can people please check
> this when reviewing?

If we need to pick a zsmalloc commit, then this probably can be related
to zs_obj_read_sg_begin() API (which is used only by zswap at the moemnt),
so probably the Fixes: tag be dc2e4982cb018 (zsmalloc: introduce SG-list
based object read API).