From patchwork Sun Jan 16 15:58:30 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Artem Bityutskiy X-Patchwork-Id: 79090 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from canuck.infradead.org (canuck.infradead.org [134.117.69.58]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 58628B6EF2 for ; Mon, 17 Jan 2011 03:01:42 +1100 (EST) Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1PeV0A-0003WZ-Fv; Sun, 16 Jan 2011 15:58:46 +0000 Received: from mail-fx0-f49.google.com ([209.85.161.49]) by canuck.infradead.org with esmtp (Exim 4.72 #1 (Red Hat Linux)) id 1PeV03-0003Uj-0O for linux-mtd@lists.infradead.org; Sun, 16 Jan 2011 15:58:40 +0000 Received: by fxm19 with SMTP id 19so4904565fxm.36 for ; Sun, 16 Jan 2011 07:58:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:subject:from:reply-to:to:cc:in-reply-to :references:content-type:date:message-id:mime-version:x-mailer :content-transfer-encoding; bh=BVADUd56RIvlAkBFb2NNzqnpnvb2qvgyjUOUiv2HQ/I=; b=FTTgdF0iCrvAaxNPkHEWToK/+Y4/Ez6lhiBi4y1eM8Xjt31kIm0+hPlXGGoQEt0ghk fBJSlfM2TR8xaHbdxlHNLgds+iIDD4A0rHglczzO9/8OeJ1HdkJpccFUBKshMspnFN2e bR7+Ed5XpusCy7cH7A8IoKE3K1mUtaSkF3eJ8= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:reply-to:to:cc:in-reply-to:references:content-type :date:message-id:mime-version:x-mailer:content-transfer-encoding; b=MTii7/AH/LC7ur3qG3mP5e1MAzhUWP1bmYJlC2eTh+HVUYOUluss3gGMiaB1n8DbJA /aTGXb/WPUd2jbBxFXrnI43GfUvvc5qu1buTrWP7nXSITWjZh3gPZ+kjmzvfScCUQzOQ SbjXavf0q5M7UBnoX8k4EHSNrR3X5ishcqLag= Received: by 10.223.123.142 with SMTP id p14mr3559293far.56.1295193514119; Sun, 16 Jan 2011 07:58:34 -0800 (PST) Received: from [192.168.255.4] (a91-152-69-107.elisa-laajakaista.fi [91.152.69.107]) by mx.google.com with ESMTPS id l14sm1267873fan.9.2011.01.16.07.58.31 (version=SSLv3 cipher=RC4-MD5); Sun, 16 Jan 2011 07:58:31 -0800 (PST) Subject: Re: [PATCH 1/1] mtd: nand: add check for out of page read From: Artem Bityutskiy To: Jason Liu In-Reply-To: References: <1290156045-11719-1-git-send-email-r64343@freescale.com> <1292340409.2538.40.camel@localhost> <1292777645.13362.3.camel@koala> Date: Sun, 16 Jan 2011 17:58:30 +0200 Message-ID: <1295193510.2470.2.camel@koala> Mime-Version: 1.0 X-Mailer: Evolution 2.32.1 (2.32.1-1.fc14) X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20110116_105839_423362_BE4C41C0 X-CRM114-Status: GOOD ( 27.84 ) X-Spam-Score: 1.4 (+) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (1.4 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [209.85.161.49 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is freemail (dedekind1[at]gmail.com) 2.2 FREEMAIL_ENVFROM_END_DIGIT Envelope-from freemail username ends in digit (dedekind1[at]gmail.com) -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature Cc: Jason Liu , linux-mtd@lists.infradead.org, David.Woodhouse@intel.com, linux-kernel@vger.kernel.org X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: dedekind1@gmail.com List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org On Thu, 2010-12-23 at 14:06 +0800, Jason Liu wrote: > Hi, Artem, > > 2010/12/20 Artem Bityutskiy : > > On Wed, 2010-12-15 at 09:55 +0800, Jason Liu wrote: > >> > /* Do not allow reads past end of device */ > >> > if (unlikely(from >= mtd->size || > >> > ops->ooboffs + readlen > ((mtd->size >> chip->page_shift) - > >> > (from >> chip->page_shift)) * len)) { > >> > DEBUG(MTD_DEBUG_LEVEL0, "%s: Attempt read beyond end " > >> > "of device\n", __func__); > >> > return -EINVAL; > >> > } > >> > >> Here the mtd->size in nand_base.c should be the NAND flash chip size, > > > > I think this is partition size as well. > > I think you are wrong. This should be the NAND flash chip size not the > partition size. Yes, thanks for pointing this. The patch below fixes the issue you found. I've also pushed this patch to my l2-mtd-2.6 tree. If you can test it, please, do, but it fixes the issue when nandsim is used. From 2b6dc448005afdfe48ac2096d24694b8ea3ac88b Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Sun, 16 Jan 2011 17:50:54 +0200 Subject: [PATCH] mtd: mtdpart: disallow reading OOB past the end of the partition This patch fixes the mtdpart bug which allows users reading OOB past the end of the partition. This happens because 'part_read_oob()' allows reading multiple OOB areas in one go, and mtdparts does not validate the OOB length in the request. Although there is such check in 'nand_do_read_oob()' in nand_base.c, but it checks that we do not read past the flash chip, not the partition, because in nand_base.c we work with the whole chip (e.g., mtd->size in nand_base.c is the size of the whole chip). So this check cannot be done correctly in nand_base.c and should be instead done in mtdparts.c. This problem was reported by Jason Liu and reproduced with nandsim: $ modprobe nandsim first_id_byte=0x20 second_id_byte=0xaa third_id_byte=0x00 \ fourth_id_byte=0x15 parts=0x400,0x400 $ modprobe nandsim mtd_oobtest.ko dev=0 $ dmesg = snip = mtd_oobtest: attempting to read past end of device mtd_oobtest: an error is expected... mtd_oobtest: error: read past end of device = snip = mtd_oobtest: finished with 2 errors Reported-by: Jason Liu Signed-off-by: Artem Bityutskiy --- drivers/mtd/mtdpart.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-) diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 79e3689..d2b932f 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -120,8 +120,25 @@ static int part_read_oob(struct mtd_info *mtd, loff_t from, return -EINVAL; if (ops->datbuf && from + ops->len > mtd->size) return -EINVAL; - res = part->master->read_oob(part->master, from + part->offset, ops); + /* + * If OOB is also requested, make sure that we do not read past the end + * of this partition. + */ + if (ops->oobbuf) { + size_t len, pages; + + if (ops->mode == MTD_OOB_AUTO) + len = mtd->oobavail; + else + len = mtd->oobsize; + pages = mtd_div_by_ws(mtd->size, mtd); + pages -= mtd_div_by_ws(from, mtd); + if (ops->ooboffs + ops->ooblen > pages * len) + return -EINVAL; + } + + res = part->master->read_oob(part->master, from + part->offset, ops); if (unlikely(res)) { if (res == -EUCLEAN) mtd->ecc_stats.corrected++;