From patchwork Tue Oct 27 22:16:43 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rasmus Villemoes X-Patchwork-Id: 7504831 Return-Path: X-Original-To: patchwork-linux-scsi@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id B21F2BEEA4 for ; Tue, 27 Oct 2015 22:17:29 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A30A3209BD for ; Tue, 27 Oct 2015 22:17:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6F0D2209B5 for ; Tue, 27 Oct 2015 22:17:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754966AbbJ0WRH (ORCPT ); Tue, 27 Oct 2015 18:17:07 -0400 Received: from mail-lf0-f52.google.com ([209.85.215.52]:32998 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754947AbbJ0WRA (ORCPT ); Tue, 27 Oct 2015 18:17:00 -0400 Received: by lffv3 with SMTP id v3so179342561lff.0 for ; Tue, 27 Oct 2015 15:16:58 -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=GLwWI+TW07Op4JRl+++sg6wX+46FRKgkCLPOnSdgDBk=; b=gher2nX8NO/HEsUXF1P+OiFQwk8/OZulrO0ZwSTQZ0Js1Z5OZBZc+lzWI0R8hT12J0 2BAyOGNUJ+Hcd16ZaaD7Uklc6zCU92yGT2JR3rGyMw49vUIr2+eIRJX6ktCz+KJfp07T xqDv7p/fzO6/lUnb8N6F8Q7lXVcD54x0+CPo0= 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=GLwWI+TW07Op4JRl+++sg6wX+46FRKgkCLPOnSdgDBk=; b=mqKkXImZy1z8KgCPqdvJbpolPk3hXmqILEJsXpdBWmAUuL3VVFBfyjiMWGZbt7qi6q iJq4OutHgXvvnrYVA2cnG5b9tLLuyk3p+v2ccUUYuD8fT0kBIcdoUNdGb8gaAN3OqxbM 9CzxJiGiZmSAvsBeiSKSImkhlLwVK7pLQz5rF4REco3BJupCyk991ER8A2eTBwwyo4ad pYzH1psdbL7DzrC4V55NC7IzDKMWWbHAK1++y6onu0MFCYE8MOvEKEpM9A9uTJNo4qp6 P8Us9zUv+7EG8xz+O9iCAZVnQYgsjD9yVqPWpc0m975imKf26lXZ5Q230zniFi6NI3UG 7Sfw== X-Gm-Message-State: ALoCoQmyadRDbi+pN/Nze5nDLlRAKLQj7PAOLN/1BTGz+vDyJMo4JodIMPPshnGfqgh/cmRazrqe X-Received: by 10.25.158.211 with SMTP id h202mr7695538lfe.29.1445984218658; Tue, 27 Oct 2015 15:16:58 -0700 (PDT) Received: from garcia.imf.au.dk ([130.225.20.51]) by smtp.gmail.com with ESMTPSA id dt9sm7277948lbc.38.2015.10.27.15.16.57 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 27 Oct 2015 15:16:58 -0700 (PDT) From: Rasmus Villemoes To: Joe Handzik , Don Brace , "James E.J. Bottomley" Cc: Kevin Barnett , Scott Teel , Tomas Henzl , Rasmus Villemoes , iss_storagedev@hp.com, storagedev@pmcs.com, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] scsi: hpsa: fix multiple issues in path_info_show Date: Tue, 27 Oct 2015 23:16:43 +0100 Message-Id: <1445984203-3288-1-git-send-email-linux@rasmusvillemoes.dk> X-Mailer: git-send-email 2.6.1 Sender: linux-scsi-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-scsi@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP I'm not familiar with this code, but path_info_show() (added in 8270b86243658 "hpsa: add sysfs entry path_info to show box and bay information") seems to be broken in multiple ways. First, there's 817 return snprintf(buf, output_len+1, "%s%s%s%s%s%s%s%s", 818 path[0], path[1], path[2], path[3], 819 path[4], path[5], path[6], path[7]); so hopefully output_len contains the combined length of the eight strings. Otherwise, snprintf will stop copying to the output buffer, but still end up reporting that combined length - which in turn would result in user-space getting a bunch of useless nul bytes (thankfully the upper sysfs layer seems to clear the output buffer before passing it to the various ->show routines). But we have 767 output_len = snprintf(path[i], 768 PATH_STRING_LEN, "[%d:%d:%d:%d] %20.20s ", 769 h->scsi_host->host_no, 770 hdev->bus, hdev->target, hdev->lun, 771 scsi_device_type(hdev->devtype)); so output_len at best contains the length of the last string printed. Inside the loop, we then otherwise add to output_len. By magic, we still have PATH_STRING_LEN available every time... This wouldn't really be a problem if the bean-counting has been done properly and each line actually does fit in 50 bytes, and maybe it does, but I don't immediately see why. Suppose we end up taking this branch: 802 output_len += snprintf(path[i] + output_len, 803 PATH_STRING_LEN, 804 "BOX: %hhu BAY: %hhu %s\n", 805 box, bay, active); An optimistic estimate says this uses strlen("BOX: 1 BAY: 2 Active\n") which is 21. Now add the 20 bytes guaranteed by the %20.20s and then some for the rest of that format string, and we're easily over 50 bytes. I don't think we can get over 100 bytes even being pessimistic, so this just means we'll scribble into the next path[i+1] and maybe get that overwritten later, leading to some garbled output (in fact, since we'd overwrite the previous string's 0-terminator, we could end up with one very long string and then print various suffixes of that, leading to much more than 400 bytes of output). Except of course when we're filling path[7], where overrunning it means writing random stuff to the kernel stack, which is usually a lot of fun. We can fix all of that and get rid of the 400 byte stack buffer by simply writing directly to the given output buffer, which the upper layer guarantees is at least PAGE_SIZE. s[c]nprintf doesn't care where it is writing to, so this doesn't make the spin lock hold time any longer. Using scnprintf ensures that output_len always represents the number of bytes actually written to the buffer, so we'll report the proper amount to the upper layer. Signed-off-by: Rasmus Villemoes --- drivers/scsi/hpsa.c | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 40669f8dd0df..b892ae90e292 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -726,7 +726,6 @@ static ssize_t host_show_hp_ssd_smart_path_enabled(struct device *dev, } #define MAX_PATHS 8 -#define PATH_STRING_LEN 50 static ssize_t path_info_show(struct device *dev, struct device_attribute *attr, char *buf) @@ -742,9 +741,7 @@ static ssize_t path_info_show(struct device *dev, u8 path_map_index = 0; char *active; unsigned char phys_connector[2]; - unsigned char path[MAX_PATHS][PATH_STRING_LEN]; - memset(path, 0, MAX_PATHS * PATH_STRING_LEN); sdev = to_scsi_device(dev); h = sdev_to_hba(sdev); spin_lock_irqsave(&h->devlock, flags); @@ -764,8 +761,9 @@ static ssize_t path_info_show(struct device *dev, else continue; - output_len = snprintf(path[i], - PATH_STRING_LEN, "[%d:%d:%d:%d] %20.20s ", + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, + "[%d:%d:%d:%d] %20.20s ", h->scsi_host->host_no, hdev->bus, hdev->target, hdev->lun, scsi_device_type(hdev->devtype)); @@ -773,9 +771,9 @@ static ssize_t path_info_show(struct device *dev, if (is_ext_target(h, hdev) || (hdev->devtype == TYPE_RAID) || is_logical_dev_addr_mode(hdev->scsi3addr)) { - output_len += snprintf(path[i] + output_len, - PATH_STRING_LEN, "%s\n", - active); + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, + "%s\n", active); continue; } @@ -787,36 +785,34 @@ static ssize_t path_info_show(struct device *dev, if (phys_connector[1] < '0') phys_connector[1] = '0'; if (hdev->phys_connector[i] > 0) - output_len += snprintf(path[i] + output_len, - PATH_STRING_LEN, + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, "PORT: %.2s ", phys_connector); if (hdev->devtype == TYPE_DISK && hdev->expose_state != HPSA_DO_NOT_EXPOSE) { if (box == 0 || box == 0xFF) { - output_len += snprintf(path[i] + output_len, - PATH_STRING_LEN, + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, "BAY: %hhu %s\n", bay, active); } else { - output_len += snprintf(path[i] + output_len, - PATH_STRING_LEN, + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, "BOX: %hhu BAY: %hhu %s\n", box, bay, active); } } else if (box != 0 && box != 0xFF) { - output_len += snprintf(path[i] + output_len, - PATH_STRING_LEN, "BOX: %hhu %s\n", + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, "BOX: %hhu %s\n", box, active); } else - output_len += snprintf(path[i] + output_len, - PATH_STRING_LEN, "%s\n", active); + output_len += scnprintf(buf + output_len, + PAGE_SIZE - output_len, "%s\n", active); } spin_unlock_irqrestore(&h->devlock, flags); - return snprintf(buf, output_len+1, "%s%s%s%s%s%s%s%s", - path[0], path[1], path[2], path[3], - path[4], path[5], path[6], path[7]); + return output_len; } static DEVICE_ATTR(raid_level, S_IRUGO, raid_level_show, NULL);