From patchwork Mon Sep 15 17:31:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Norris X-Patchwork-Id: 389446 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from bombadil.infradead.org (bombadil.infradead.org [IPv6:2001:1868:205::9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id CF71314018C for ; Tue, 16 Sep 2014 03:34:24 +1000 (EST) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1XTa8f-0003oD-3J; Mon, 15 Sep 2014 17:32:33 +0000 Received: from mail-pa0-x22e.google.com ([2607:f8b0:400e:c03::22e]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1XTa8W-0003jB-Sn for linux-mtd@lists.infradead.org; Mon, 15 Sep 2014 17:32:28 +0000 Received: by mail-pa0-f46.google.com with SMTP id kq14so6873101pab.5 for ; Mon, 15 Sep 2014 10:32:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=B1HIhGvdY7FOxS6qakRcQLutXljImd8ralm6VbFnWKY=; b=QA12pg25nh/s+ZsU002HU7tEoKCfudO6DDBgcsx2ehs2TzrVWcIy0Kh/zvjoO6AlMO ducGIJwtmcaYQdGtFfIIv0tOEZOVr1GOCZxQXcld8bodfvfMxjs6x6wBihPfJi4yDTuZ GrAWzqAggCjNR+/DPCKBLZm5Hj7TJwdTaJlZUlNt1Zoy1dHzMq/8pas+cdddu8wQvy3u 8HfgU7l1AKSXJDjvbKg2XzCe/LZ0Vb7lVo+ilns74t4uypZncKqZ0VviiUBkloI8VRw9 +GSehJfHlEhTSl7vF68YuPttAibj0dWUOKxERBbpdvnZzOhs6k4LgM7+KCWxPdvbtwbb Dlkw== X-Received: by 10.66.217.166 with SMTP id oz6mr40496802pac.23.1410802321091; Mon, 15 Sep 2014 10:32:01 -0700 (PDT) Received: from ld-irv-0074 (5520-maca-inet1-outside.broadcom.com. [216.31.211.11]) by mx.google.com with ESMTPSA id k8sm11930048pdo.64.2014.09.15.10.31.59 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 15 Sep 2014 10:32:00 -0700 (PDT) Date: Mon, 15 Sep 2014 10:31:52 -0700 From: Brian Norris To: Wei.Yang@windriver.com Subject: Re: [PATCH v1] mtdinfo: Initialize reginfo variable before invoking mtd_regioninfo Message-ID: <20140915173152.GA14832@ld-irv-0074> References: <1407294015-27884-1-git-send-email-Wei.Yang@windriver.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1407294015-27884-1-git-send-email-Wei.Yang@windriver.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20140915_103224_954702_BAED06DD X-CRM114-Status: GOOD ( 23.56 ) X-Spam-Score: -0.8 (/) X-Spam-Report: SpamAssassin version 3.4.0 on bombadil.infradead.org summary: Content analysis details: (-0.8 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [2607:f8b0:400e:c03:0:0:0:22e listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (computersforpeace[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: linux-mtd@lists.infradead.org, w90p710@gmail.com, dedekind1@gmail.com X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-mtd" Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org Hi Yang, On Wed, Aug 06, 2014 at 11:00:15AM +0800, Wei.Yang@windriver.com wrote: > From: Yang Wei > > If not initializing this variable, its value would be random. So > once the value of the regionindex field of region_info_user structure > is greater than mtd->numeraseregions. ioctl,which comes with MEMGETREGIONINFO, > would return -EINVAL > > Signed-off-by: Yang Wei > --- > ubi-utils/mtdinfo.c | 2 ++ > 1 file changed, 2 insertions(+) > > Hi Guys, > > I got the following errors when running "mtdinfo /dev/mtd0" on Cavium 6100 board, > > > root@CN61XX:~# mtdinfo /dev/mtd0 > mtd0 > Name: phys_mapped_flash > Type: nor > Eraseblock size: 65536 bytes, 64.0 KiB > Amount of eraseblocks: 128 (8388608 bytes, 8.0 MiB) > Minimum input/output unit size: 1 byte > Sub-page size: 1 byte > Additional erase regions: 0 > Character device major/minor: 90:0 > Bad blocks are allowed: false > Device is writable: true > libmtd: error!: MEMGETREGIONINFO ioctl failed for erase region 0 > error 22 (Invalid argument) > Eraseblock region 0: info is unavailable > libmtd: error!: MEMGETREGIONINFO ioctl failed for erase region 1 > error 22 (Invalid argument) > Eraseblock region 1: info is unavailable These error messages look deceptive. The problem is that while mtd_regioninfo() takes a region index parameter, it only uses it for printing the error message; its value is not actually propagated to the ioctl(). > > This patch is to fix the above errors. I have already verified it > > root@CN61XX:~# ./mtdinfo /dev/mtd0 > mtd0 > Name: phys_mapped_flash > Type: nor > Eraseblock size: 65536 bytes, 64.0 KiB > Amount of eraseblocks: 128 (8388608 bytes, 8.0 MiB) > Minimum input/output unit size: 1 byte > Sub-page size: 1 byte > Additional erase regions: 0 > Character device major/minor: 90:0 > Bad blocks are allowed: false > Device is writable: true > Eraseblock region 0: offset: 0 size: 0x10000 numblocks: 0x7f > Eraseblock region 1: offset: 0 size: 0x10000 numblocks: 0x7f These last two lines look wrong. Why would the device have two identical erase regions? In fact, it looks like your patch just means that we'll call ioctl(MEMGETREGIONINFO) repeatedly for region 0, instead of once for each indexed region. > > root@CN61XX:~# > > > diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c > index 5ac95aa..a70db00 100644 > --- a/ubi-utils/mtdinfo.c > +++ b/ubi-utils/mtdinfo.c > @@ -253,6 +253,8 @@ static void print_region_info(const struct mtd_dev_info *mtd) > if (!args.node || (!args.map && mtd->region_cnt == 0)) > return; > > + memset(®info, 0, sizeof(region_info_t)); > + This might be good for security (to make sure that we never leak garbage or use garbage stack data), but it doesn't actually fix anything. > /* First open the device so we can query it */ > fd = open(args.node, O_RDONLY | O_CLOEXEC); > if (fd == -1) { I think you need a patch like this instead (untested): Brian diff --git a/lib/libmtd.c b/lib/libmtd.c index aff4c8be01b5..a6665f08018e 100644 --- a/lib/libmtd.c +++ b/lib/libmtd.c @@ -901,6 +901,8 @@ int mtd_regioninfo(int fd, int regidx, struct region_info_user *reginfo) return -1; } + reginfo->regionindex = regidx; + ret = ioctl(fd, MEMGETREGIONINFO, reginfo); if (ret < 0) return sys_errmsg("%s ioctl failed for erase region %d", diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c index 5ac95aaabb8a..cd61105c5c01 100644 --- a/ubi-utils/mtdinfo.c +++ b/ubi-utils/mtdinfo.c @@ -253,6 +253,9 @@ static void print_region_info(const struct mtd_dev_info *mtd) if (!args.node || (!args.map && mtd->region_cnt == 0)) return; + /* Just in case */ + memset(®info, 0, sizeof(reginfo)); + /* First open the device so we can query it */ fd = open(args.node, O_RDONLY | O_CLOEXEC); if (fd == -1) {