Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753931AbdHXQvi (ORCPT <rfc822;w@1wt.eu>);
        Thu, 24 Aug 2017 12:51:38 -0400
Received: from mail.kernel.org ([198.145.29.99]:48678 "EHLO mail.kernel.org"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1753411AbdHXQvg (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 24 Aug 2017 12:51:36 -0400
DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org F263721A1B
Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org
Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=robh+dt@kernel.org
MIME-Version: 1.0
In-Reply-To: <20170824010352.9085-1-bjorn.andersson@linaro.org>
References: <20170824010352.9085-1-bjorn.andersson@linaro.org>
From: Rob Herring <robh+dt@kernel.org>
Date: Thu, 24 Aug 2017 11:51:14 -0500
X-Gmail-Original-Message-ID: <CAL_JsqLqc488ALEqZT-PKw54h5QAbU=q=CEhM9jwROmukR7fZg@mail.gmail.com>
Message-ID: <CAL_JsqLqc488ALEqZT-PKw54h5QAbU=q=CEhM9jwROmukR7fZg@mail.gmail.com>
Subject: Re: [PATCH] of/device: Fix of_device_get_modalias() buffer handling
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Frank Rowand <frowand.list@gmail.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3327
Lines: 86

On Wed, Aug 23, 2017 at 8:03 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> of_device_request_module() calls of_device_get_modalias() with "len" 0,
> to calculate the size of the buffer needed to store the result, but due
> to integer promotion the ssize_t "len" will be compared as unsigned with
> strlen(compat) and the loop will generally never break. This results in
> a call to snprintf() with a negative len, which triggers below warning,
> followed by a dereference of a invalid pointer:
>
>   [    3.060067] WARNING: CPU: 0 PID: 51 at lib/vsprintf.c:2122 vsnprintf+0x348/0x6d8
>   ...
>   [    3.060301] [<ffffff800891ede8>] vsnprintf+0x348/0x6d8
>   [    3.060308] [<ffffff800891f248>] snprintf+0x48/0x50
>   [    3.060316] [<ffffff80086a7c80>] of_device_get_modalias+0x108/0x160
>   [    3.060322] [<ffffff80086a7cf8>] of_device_request_module+0x20/0x88
>   ...
>
> Further more of_device_get_modalias() is supposed to return the number
> of bytes needed to store the entire modalias, so the loop needs to
> continue accumulate the total size even though the buffer is full.
>
> Finally the function is not expected to ensure space for the NUL, nor
> include it in the returned size, so only 1 should be added to the length
> of "compat" in the loop (to account for the character 'C').
>
> Fixes: bc575064d688 ("of/device: use of_property_for_each_string to parse compatible strings")
> Cc: Rob Herring <robh@kernel.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/of/device.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 6f33a0e0d351..7cff599a9c6a 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -195,10 +195,11 @@ EXPORT_SYMBOL(of_device_get_match_data);
>
>  static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len)
>  {
> -       const char *compat, *start = str;
> +       const char *compat;
>         char *c;
>         struct property *p;
>         ssize_t csize;
> +       ssize_t tsize;
>
>         if ((!dev) || (!dev->of_node))
>                 return -ENODEV;
> @@ -206,12 +207,16 @@ static ssize_t of_device_get_modalias(struct device *dev, char *str, ssize_t len
>         /* Name & Type */
>         csize = snprintf(str, len, "of:N%sT%s", dev->of_node->name,
>                          dev->of_node->type);
> +       tsize = csize;
>         len -= csize;
> -       str += csize;
> +       if (str)
> +               str += csize;
>
>         of_property_for_each_string(dev->of_node, "compatible", p, compat) {
> -               if (strlen(compat) + 2 > len)
> -                       break;
> +               csize = strlen(compat) + 1;
> +               tsize += csize;
> +               if (csize > len)
> +                       continue;
>
>                 csize = snprintf(str, len, "C%s", compat);

We could just use the snprintf to give us the length. Something like
this following the snprintf:

tsize +=csize;
if (csize > len) {
  if (len) {
    str[0] = '\0';
    len = 0;
  }
  continue;
}

You'd need to prevent len from going negative up above too. It ends up
being more lines but you save a strlen call. So perhaps it's fine as
is, but since I've written it already throwing it out there...

Rob