Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752912Ab3FEIxG (ORCPT ); Wed, 5 Jun 2013 04:53:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49189 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752208Ab3FEIxD (ORCPT ); Wed, 5 Jun 2013 04:53:03 -0400 Message-ID: <51AEFC60.70107@redhat.com> Date: Wed, 05 Jun 2013 10:52:48 +0200 From: Jerome Marchand User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Jiang Liu CC: Greg Kroah-Hartman , Nitin Gupta , Minchan Kim , Yijing Wang , Jiang Liu , devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH v1 6/8] zram: avoid access beyond the zram device References: <1370274140-26420-1-git-send-email-jiang.liu@huawei.com> <1370274140-26420-7-git-send-email-jiang.liu@huawei.com> <51ADE87F.9080303@redhat.com> <51AE033D.8090302@gmail.com> In-Reply-To: <51AE033D.8090302@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1970 Lines: 64 On 06/04/2013 05:09 PM, Jiang Liu wrote: > On Tue 04 Jun 2013 09:15:43 PM CST, Jerome Marchand wrote: >> On 06/03/2013 05:42 PM, Jiang Liu wrote: >>> Function valid_io_request() should verify the entire request doesn't >>> exceed the zram device, otherwise it will cause invalid memory access. >>> >>> Signed-off-by: Jiang Liu >>> --- >>> drivers/staging/zram/zram_drv.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c >>> index 66cf28a..64b51b9 100644 >>> --- a/drivers/staging/zram/zram_drv.c >>> +++ b/drivers/staging/zram/zram_drv.c >>> @@ -428,6 +428,10 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio) >>> return 0; >>> } >>> >>> + if (unlikely((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >= >>> + zram->disksize)) >>> + return 0; >>> + >> >> This test make the first line of previous test redundant. Why not just >> update it like the following: >> >> - (bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) || >> + ((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >= >> + zram->disksize)) || >> >> >> Jerome > Hi Jerome, > I think the test "bio->bi_sector >= (zram->disksize >> > SECTOR_SHIFT)" is still > needed to protect "(bio->bi_sector << SECTOR_SHIFT) + bio->bi_size" > from wrapping > around. Good point, but I don't see how this is going to catch all the possible values that overflow. You still need an explicit overflow test (bio->bi_sector << SECTOR_SHIFT) + bio->bi_size < bio->bi_size), at which point the first test would be useless. Jerome > Regards! > Gerry > >> >>> /* I/O request is valid */ >>> return 1; >>> } >>> >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/