From patchwork Tue Aug 9 15:52:38 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Carpenter X-Patchwork-Id: 1050182 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter1.kernel.org (8.14.4/8.14.4) with ESMTP id p79FuoAL025075 for ; Tue, 9 Aug 2011 15:56:50 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753824Ab1HIP4r (ORCPT ); Tue, 9 Aug 2011 11:56:47 -0400 Received: from mail-yw0-f46.google.com ([209.85.213.46]:46419 "EHLO mail-yw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753028Ab1HIP4q (ORCPT ); Tue, 9 Aug 2011 11:56:46 -0400 Received: by ywf7 with SMTP id 7so93012ywf.19 for ; Tue, 09 Aug 2011 08:56:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:mime-version:content-type :content-disposition:user-agent; bh=5XYQrpIcbg9lSkUWyN6Nmy8BhyV561cdmPbAO7Nxf2w=; b=kqAXlYFTjupmpIUIS8QyT4Wm9Dvot+uahY1ZjkZHN1A1A2gYbVc2EA14/e0Hknna5k 8/U8Z9+hXNQQpCefjxuIevB6YfQXexw37spMJKlV7V8bg2xls6npJyOmGTBt+Lr/F1p9 KrO8h5B8W2kAHX5QATTvdhurFEZ7ZbatM3ciY= Received: by 10.142.2.12 with SMTP id 12mr5754603wfb.247.1312905405269; Tue, 09 Aug 2011 08:56:45 -0700 (PDT) Received: from shale.localdomain ([41.139.221.94]) by mx.google.com with ESMTPS id v1sm74236pbg.15.2011.08.09.08.56.40 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 09 Aug 2011 08:56:44 -0700 (PDT) Date: Tue, 9 Aug 2011 18:52:38 +0300 From: Dan Carpenter To: Mauro Carvalho Chehab Cc: Oliver Endriss , Ralph Metzler , "open list:MEDIA INPUT INFRA..." , kernel-janitors@vger.kernel.org Subject: [patch] [media] ddbridge: fix ddb_ioctl() Message-ID: <20110809155238.GA3830@shale.localdomain> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 09 Aug 2011 15:56:51 +0000 (UTC) There were a several problems in this function: 1) Potential integer overflow in the comparison: if (fio.write_len + fio.read_len > 1028) { 2) If the user gave bogus values for write_len and read_len then returning -EINVAL is more appropriate than returning -ENOMEM. 3) wbuf was set to the address of an array and could never be NULL so I removed the pointless NULL check. 4) The call to vfree(wbuf) was improper. That array is part of a larger struct and isn't allocated by itself. 5) flashio() can't actually fail, but we may as well add error handling in case this changes later. 6) In the default case where an ioctl is not implemented then returning -ENOTTY is more appropriate than returning -EFAULT. Signed-off-by: Dan Carpenter --- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/media/dvb/ddbridge/ddbridge-core.c b/drivers/media/dvb/ddbridge/ddbridge-core.c index 573d540..fe56703 100644 --- a/drivers/media/dvb/ddbridge/ddbridge-core.c +++ b/drivers/media/dvb/ddbridge/ddbridge-core.c @@ -1438,7 +1438,7 @@ static long ddb_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { struct ddb *dev = file->private_data; void *parg = (void *)arg; - int res = -EFAULT; + int res; switch (cmd) { case IOCTL_DDB_FLASHIO: @@ -1447,29 +1447,29 @@ static long ddb_ioctl(struct file *file, unsigned int cmd, unsigned long arg) u8 *rbuf, *wbuf; if (copy_from_user(&fio, parg, sizeof(fio))) - break; - if (fio.write_len + fio.read_len > 1028) { - printk(KERN_ERR "IOBUF too small\n"); - return -ENOMEM; - } + return -EFAULT; + + if (fio.write_len > 1028 || fio.read_len > 1028) + return -EINVAL; + if (fio.write_len + fio.read_len > 1028) + return -EINVAL; + wbuf = &dev->iobuf[0]; - if (!wbuf) - return -ENOMEM; rbuf = wbuf + fio.write_len; - if (copy_from_user(wbuf, fio.write_buf, fio.write_len)) { - vfree(wbuf); - break; - } - res = flashio(dev, wbuf, fio.write_len, - rbuf, fio.read_len); + + if (copy_from_user(wbuf, fio.write_buf, fio.write_len)) + return -EFAULT; + res = flashio(dev, wbuf, fio.write_len, rbuf, fio.read_len); + if (res) + return res; if (copy_to_user(fio.read_buf, rbuf, fio.read_len)) - res = -EFAULT; + return -EFAULT; break; } default: - break; + return -ENOTTY; } - return res; + return 0; } static const struct file_operations ddb_fops = {