From patchwork Sat Oct 17 20:58:19 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 531869 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 5E7351402B2 for ; Sun, 18 Oct 2015 08:00:00 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=rasmusvillemoes.dk header.i=@rasmusvillemoes.dk header.b=Bz0GM7Ly; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751348AbbJQU7N (ORCPT ); Sat, 17 Oct 2015 16:59:13 -0400 Received: from mail-lb0-f181.google.com ([209.85.217.181]:34406 "EHLO mail-lb0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750851AbbJQU7M (ORCPT ); Sat, 17 Oct 2015 16:59:12 -0400 Received: by lbbwb3 with SMTP id wb3so58440583lbb.1 for ; Sat, 17 Oct 2015 13:59:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rasmusvillemoes.dk; s=google; h=from:to:cc:subject:date:message-id; bh=xJdyKP6IBSAzwhE66a2D0qJxVSINiKsSxustJAHnA3U=; b=Bz0GM7LyQV1Enr9vGjt1k3D1Lc3uE2CTDMYHZxFuB6tEEMdUztdygHJFt7vQbdOZxU ONkj2o6s3BZDxkE0k5TCHtamm/wWJ+vRFc8JsR9x5HmceBfgli36sOsWr66KzDtJLZqS ZobMlF4bgxOK/kr6M/9fnZy8n9qDuCDYuEpI0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=xJdyKP6IBSAzwhE66a2D0qJxVSINiKsSxustJAHnA3U=; b=G8T1pozw2AqbNkdN6K1JOGB7jenmsZSiJtrJR5qqdd5rOCN6Mzr0mU+Yde6qNcY17d OoKra09v0awkZ2bhblzBc3bHl4hbp25Pdm3eF0eVhxPjftRJPuAL0JB3JtcQZ9K+ZGor 62UrlAjA31pXKSSkA2IynPIUmXoevQKJ+cmN8PpjbrowGQiBKyAW+LkerqfGZeHta6GZ cbsQH8mNamQNH++BxRXGfR+XUfEFe1qHhFNr92gvUk1aWaemao9JZh3d44naoZlX/RAN PzDymzdAQv3Jz4Bktu20gPoDM8gjMLFp109r2nfqLebCHVESIe8oxQDtgdOLw82KBk5Y +HMg== X-Gm-Message-State: ALoCoQk6RUfSMaymmVLDLWuwLLtU3bfFyA6uxGO9rs8BYJEIvU938mK0yCngZAvrrjawIxpgNJSW X-Received: by 10.112.135.233 with SMTP id pv9mr5337612lbb.42.1445115550712; Sat, 17 Oct 2015 13:59:10 -0700 (PDT) Received: from garcia.imf.au.dk ([130.225.20.51]) by smtp.gmail.com with ESMTPSA id g140sm3964351lfg.29.2015.10.17.13.59.09 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 17 Oct 2015 13:59:09 -0700 (PDT) From: Rasmus Villemoes To: Jeff Kirsher , Jesse Brandeburg , Shannon Nelson , Carolyn Wyborny , Don Skidmore , Matthew Vick , John Ronciak , Mitch Williams Cc: Rasmus Villemoes , intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] intel: i40e: fix confused code Date: Sat, 17 Oct 2015 22:58:19 +0200 Message-Id: <1445115499-28728-1-git-send-email-linux@rasmusvillemoes.dk> X-Mailer: git-send-email 2.6.1 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org This code is pretty confused. The variable name 'bytes_not_copied' clearly indicates that the programmer knew the semantics of copy_{to,from}_user, but then the return value is checked for being negative and used as a -Exxx return value. I'm not sure this is the proper fix, but at least we get rid of the dead code which pretended to check for access faults. Signed-off-by: Rasmus Villemoes Acked-by: Shannon Nelson --- There are other things worth looking at. i40e_dbg_netdev_ops_buf is a static buffer of size 256, which can be filled from user space (in i40e_dbg_netdev_ops_write). That function correctly checks that the input is at most 255 bytes. However, in i40e_dbg_netdev_ops_read we snprintf() it along a device name (and some white space) into kmalloc'ed buffer, also of size 256. Hence the snprintf output can be truncated, but snprintf() returns the total size that would be generated - so when we then proceed to using that in copy_to_user(), we may actually copy from beyond the allocated buffer, hence leaking a little kernel data. In i40e_dbg_command_write, we allocate a buffer based on count which is user-supplied. While kmalloc() refuses completely insane sizes, we may still allocate a few MB. Moreover, if allocation fails, returning 'count' is rather odd; -ENOMEM would make more sense. drivers/net/ethernet/intel/i40e/i40e_debugfs.c | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c index d7c15d17faa6..e9ecd3f9cafe 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_debugfs.c +++ b/drivers/net/ethernet/intel/i40e/i40e_debugfs.c @@ -103,8 +103,8 @@ static ssize_t i40e_dbg_dump_read(struct file *filp, char __user *buffer, len = min_t(int, count, (i40e_dbg_dump_data_len - *ppos)); bytes_not_copied = copy_to_user(buffer, &i40e_dbg_dump_buf[*ppos], len); - if (bytes_not_copied < 0) - return bytes_not_copied; + if (bytes_not_copied) + return -EFAULT; *ppos += len; return len; @@ -353,8 +353,8 @@ static ssize_t i40e_dbg_command_read(struct file *filp, char __user *buffer, bytes_not_copied = copy_to_user(buffer, buf, len); kfree(buf); - if (bytes_not_copied < 0) - return bytes_not_copied; + if (bytes_not_copied) + return -EFAULT; *ppos = len; return len; @@ -995,12 +995,10 @@ static ssize_t i40e_dbg_command_write(struct file *filp, if (!cmd_buf) return count; bytes_not_copied = copy_from_user(cmd_buf, buffer, count); - if (bytes_not_copied < 0) { + if (bytes_not_copied) { kfree(cmd_buf); - return bytes_not_copied; + return -EFAULT; } - if (bytes_not_copied > 0) - count -= bytes_not_copied; cmd_buf[count] = '\0'; cmd_buf_tmp = strchr(cmd_buf, '\n'); @@ -2034,8 +2032,8 @@ static ssize_t i40e_dbg_netdev_ops_read(struct file *filp, char __user *buffer, bytes_not_copied = copy_to_user(buffer, buf, len); kfree(buf); - if (bytes_not_copied < 0) - return bytes_not_copied; + if (bytes_not_copied) + return -EFAULT; *ppos = len; return len; @@ -2068,10 +2066,8 @@ static ssize_t i40e_dbg_netdev_ops_write(struct file *filp, memset(i40e_dbg_netdev_ops_buf, 0, sizeof(i40e_dbg_netdev_ops_buf)); bytes_not_copied = copy_from_user(i40e_dbg_netdev_ops_buf, buffer, count); - if (bytes_not_copied < 0) - return bytes_not_copied; - else if (bytes_not_copied > 0) - count -= bytes_not_copied; + if (bytes_not_copied) + return -EFAULT; i40e_dbg_netdev_ops_buf[count] = '\0'; buf_tmp = strchr(i40e_dbg_netdev_ops_buf, '\n');