From patchwork Fri Oct 14 09:42:18 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Artem Bityutskiy X-Patchwork-Id: 119754 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from merlin.infradead.org (merlin.infradead.org [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 31F12B6F9A for ; Fri, 14 Oct 2011 20:43:05 +1100 (EST) Received: from canuck.infradead.org ([2001:4978:20e::1]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1REeI2-00011O-Ub; Fri, 14 Oct 2011 09:42:55 +0000 Received: from localhost ([127.0.0.1] helo=canuck.infradead.org) by canuck.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1REeI2-000237-L3; Fri, 14 Oct 2011 09:42:54 +0000 Received: from mail-ww0-f49.google.com ([74.125.82.49]) by canuck.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1REeHz-00022o-Kh for linux-mtd@lists.infradead.org; Fri, 14 Oct 2011 09:42:52 +0000 Received: by wwg9 with SMTP id 9so592151wwg.18 for ; Fri, 14 Oct 2011 02:42:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=subject:from:reply-to:to:cc:date:in-reply-to:references :content-type:x-mailer:content-transfer-encoding:message-id :mime-version; bh=3mLvyJ9RED0tX0UAYf3fTNmY67LHbO+G21mF9n8P2IA=; b=VwiUrOB9p6+HdFLGHAMoKeSZXaJbQqs1t4f2bxcUjrxcnNcvDE7/bxy81jPHEityWw Vg2gy61ShOO0Yqcu6hEKhGR7Y3VUczvCYSxI4fpvC9dyxQwaKOSq+CHyMdiA+GgySVSn b/zfRLPYMwZPpcrNUuwqeJXBsT8m1N6P1CTRQ= Received: by 10.227.154.2 with SMTP id m2mr2662455wbw.39.1318585368166; Fri, 14 Oct 2011 02:42:48 -0700 (PDT) Received: from [127.0.0.1] (jfdmzpr01-ext.jf.intel.com. [134.134.139.70]) by mx.google.com with ESMTPS id gd6sm13193038wbb.1.2011.10.14.02.42.44 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 14 Oct 2011 02:42:46 -0700 (PDT) Subject: Re: [smatch stuff] mtd/ubi/gluebi: signedness warnings From: Artem Bityutskiy To: Dan Carpenter Date: Fri, 14 Oct 2011 12:42:18 +0300 In-Reply-To: <20111010092857.GA16165@elgon.mountain> References: <20111010092857.GA16165@elgon.mountain> X-Mailer: Evolution 3.0.3 (3.0.3-1.fc15) Message-ID: <1318585345.12351.87.camel@sauron> Mime-Version: 1.0 X-CRM114-Version: 20090807-BlameThorstenAndJenny ( TRE 0.7.6 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20111014_054251_943672_603A80DF X-CRM114-Status: GOOD ( 21.77 ) X-Spam-Score: -0.7 (/) X-Spam-Report: SpamAssassin version 3.3.1 on canuck.infradead.org summary: Content analysis details: (-0.7 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [74.125.82.49 listed in list.dnswl.org] 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (dedekind1[at]gmail.com) 0.1 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: linux-mtd@lists.infradead.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 Mon, 2011-10-10 at 12:28 +0300, Dan Carpenter wrote: > Smatch complains about some to the checks in gluebi.c > > drivers/mtd/ubi/gluebi.c +177 gluebi_read(6) warn: unsigned 'len' is never less than zero. > drivers/mtd/ubi/gluebi.c +221 gluebi_write(6) warn: unsigned 'len' is never less than zero. > drivers/mtd/ubi/gluebi.c +270 gluebi_erase(7) warn: unsigned 'instr->len' is never less than zero. > > I think probably these checks can just be removed? Can these things > wrap if we passed a huge value for instr->len? > > 270 if (instr->len < 0 || instr->addr + instr->len > mtd->size) > 271 return -EINVAL; > > I don't know enough about how these are called to say. Dan, smatch is right, that check is stupid. And yes, things can wrap. So I guess this need 2 patches - one removes the "< 0" check, the other prevents wrapping. And the code is a bit messy WRT int vs size_t. I guess we could fix the warnings with the following patch, but wrapping would need a separate fix. From ec819dd49d1891aafb1212f445c76ed8b272cdf2 Mon Sep 17 00:00:00 2001 From: Artem Bityutskiy Date: Fri, 14 Oct 2011 12:31:56 +0300 Subject: [PATCH] UBI: gluebi: remove useless checks drivers/mtd/ubi/gluebi.c +177 gluebi_read(6) warn: unsigned 'len' is never less than zero. drivers/mtd/ubi/gluebi.c +221 gluebi_write(6) warn: unsigned 'len' is never less than zero. drivers/mtd/ubi/gluebi.c +270 gluebi_erase(7) warn: unsigned 'instr->len' is never less than zero. Remove the checks. However, there is an outstanding issue that we can get huge offset and len which will wrap and pass the check against the mtd size. Reported by Dan Carpenter , spotted by smatch. Signed-off-by: Artem Bityutskiy --- drivers/mtd/ubi/gluebi.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/mtd/ubi/gluebi.c b/drivers/mtd/ubi/gluebi.c index 941bc3c..0b13fa2 100644 --- a/drivers/mtd/ubi/gluebi.c +++ b/drivers/mtd/ubi/gluebi.c @@ -171,10 +171,11 @@ static void gluebi_put_device(struct mtd_info *mtd) static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen, unsigned char *buf) { - int err = 0, lnum, offs, total_read; + int err = 0, lnum, offs; + size_t total_read; struct gluebi_device *gluebi; - if (len < 0 || from < 0 || from + len > mtd->size) + if (from < 0 || from + len > mtd->size) return -EINVAL; gluebi = container_of(mtd, struct gluebi_device, mtd); @@ -215,10 +216,11 @@ static int gluebi_read(struct mtd_info *mtd, loff_t from, size_t len, static int gluebi_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen, const u_char *buf) { - int err = 0, lnum, offs, total_written; + size_t total_written; + int err = 0, lnum, offs; struct gluebi_device *gluebi; - if (len < 0 || to < 0 || len + to > mtd->size) + if (to < 0 || len + to > mtd->size) return -EINVAL; gluebi = container_of(mtd, struct gluebi_device, mtd); @@ -265,9 +267,9 @@ static int gluebi_erase(struct mtd_info *mtd, struct erase_info *instr) int err, i, lnum, count; struct gluebi_device *gluebi; - if (instr->addr < 0 || instr->addr > mtd->size - mtd->erasesize) + if (instr->addr > mtd->size - mtd->erasesize) return -EINVAL; - if (instr->len < 0 || instr->addr + instr->len > mtd->size) + if (instr->addr + instr->len > mtd->size) return -EINVAL; if (mtd_mod_by_ws(instr->addr, mtd) || mtd_mod_by_ws(instr->len, mtd)) return -EINVAL;