linux-kernel - Re: [PATCH] zram: fix null dereference of handle

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:   Tue, 19 Sep 2017 15:59:24 +0900
From:   Minchan Kim <minchan@...nel.org>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, kernel-team <kernel-team@....com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>
Subject: Re: [PATCH] zram: fix null dereference of handle

Hi Sergey,

On Tue, Sep 19, 2017 at 02:39:35PM +0900, Sergey Senozhatsky wrote:
> On (09/19/17 11:34), Minchan Kim wrote:
> [..]
> >  static void zram_meta_free(struct zram *zram, u64 disksize)
> >  {
> >  	size_t num_pages = disksize >> PAGE_SHIFT;
> > @@ -876,11 +855,18 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
> >  		zram_slot_unlock(zram, index);
> >  	}
> >  
> > -	if (zram_same_page_read(zram, index, page, 0, PAGE_SIZE))
> > -		return 0;
> > -
> >  	zram_slot_lock(zram, index);
> >  	handle = zram_get_handle(zram, index);
> > +	if (unlikely(!handle || zram_test_flag(zram, index, ZRAM_SAME))) {
> > +		void *mem;
> 
> 
> is this branch really unlikely()? ZRAM_SAME ratio really depends,
> on some setups it can be quite likely, I suspect.

Yub. Let's drop it.

> 
> 
> another question, "!handle  ==  value & ZRAM_SAME"? if so, then why not
> just check for `flags & ZRAM_SAME'? if not then:
> 
> -  for `value & ZRAM_SAME' you fill the page with zram_get_element(zram, index)
>    and return 0. ok.
> 
> -  for !handle.... you also fill the page with zram_get_element(zram, index)
>    and return 0. is this ok? shouldn't !handle return error in this case?

We discussed it before that we shouldn't return error.
Userspace can ask reading unallocated buffer freely.

And in this case, it fills the buffer zero because handle and element is unified.
However, if your concern is readability, I will make it more explict.

> 
> 
> I really suspect that there are some paths that can lead to !handle
> entry, that will not be ZRAM_SAME. e.g. error return from compression
> path.

Could you be more specific?

Thanks for the review, Sergey!