Re: [PATCH 1 23/25] hpsa: fix multiple issues in path_info_show

[Hannes Reinecke <hare@suse.de>]

On 10/28/2015 11:06 PM, Don Brace wrote:
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> 
> path_info_show() 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 <linux@rasmusvillemoes.dk>
> Signed-off-by: Don Brace <don.brace@pmcs.com>
> ---
>  drivers/scsi/hpsa.c |   38 +++++++++++++++++---------------------
>  1 file changed, 17 insertions(+), 21 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html