From: Rob Herring <robh+dt@kernel.org>
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>
Subject: Re: [PATCH] of/device: Fix of_device_get_modalias() buffer handling
Date: Thu, 24 Aug 2017 11:51:14 -0500 [thread overview]
Message-ID: <CAL_JsqLqc488ALEqZT-PKw54h5QAbU=q=CEhM9jwROmukR7fZg@mail.gmail.com> (raw)
In-Reply-To: <20170824010352.9085-1-bjorn.andersson@linaro.org>
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
next prev parent reply other threads:[~2017-08-24 16:51 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-24 1:03 [PATCH] of/device: Fix of_device_get_modalias() buffer handling Bjorn Andersson
2017-08-24 16:51 ` Rob Herring [this message]
2017-08-24 17:25 ` Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL_JsqLqc488ALEqZT-PKw54h5QAbU=q=CEhM9jwROmukR7fZg@mail.gmail.com' \
--to=robh+dt@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=frowand.list@gmail.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).