linux-kernel - Re: [PATCH 1/3] zram: fix operator precedence to get offset

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:   Sat, 15 Apr 2017 00:33:25 +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,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        kernel-team@....com, stable@...r.kernel.org
Subject: Re: [PATCH 1/3] zram: fix operator precedence to get offset

Hi Sergey,

On Fri, Apr 14, 2017 at 02:07:47PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (04/13/17 09:17), Minchan Kim wrote:
> [..]
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 9e2199060040..83c38a123242 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -930,7 +930,7 @@ static int zram_rw_page(struct block_device *bdev, sector_t sector,
> >  	}
> >  
> >  	index = sector >> SECTORS_PER_PAGE_SHIFT;
> > -	offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > +	offset = (sector & (SECTORS_PER_PAGE - 1)) << SECTOR_SHIFT;
> 
> sorry, can it actually produce different results?

I got your point. Actually, offset was wrong but rw_page is called
with PAGE_SIZE io while that offset is related to only partial io
(non-PAGEE size io). IOW, although the wrong offset it is never used
in functions.

To find subtle corruption in ppc64, I added some debug code to
catch up wrong buffer overflow and found it with other bugs but
didn't prove the specific case is valid case or not. Good catch, Sergey!

However, it should be *fixed* to prevent confusion in future but surely,
no need to go to the stable. I will send reply to Greg to prevent merging
it to *stable* when he send review asking to merge.

And next week I will send another fix which *maybe* removes code to get the
offset in zram_rw_page.

Thanks.