From: Tadeusz Struk <tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
To: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH v2 1/2] libfdt: prevent integer overflow in fdt_next_tag
Date: Tue, 4 Oct 2022 16:06:10 -0700 [thread overview]
Message-ID: <53f2985d-15a8-d266-2fbe-ed557036ad6b@linaro.org> (raw)
In-Reply-To: <YzvbXZG5lSfDL/6v@yekko>
Hi David,
On 10/4/22 00:06, David Gibson wrote:
> On Fri, Sep 30, 2022 at 08:20:03AM -0700, Tadeusz Struk wrote:
>> Since fdt_next_tag() in a public API function all input parameters,
>> including the fdt blob should not be trusted. It is possible to forge
>> a blob with invalid property length that will cause integer overflow
>> during offset calculation. To prevent that, validate the property length
>> read from the blob before doing calculations.
> So.. yes, there can be an integer overflow here. I think the actual
> damage it can cause is strongly mitigated by the fact that we should
> only ever use the result of that overflow via fdt_offset_ptr(), which
> will safely reject a bad offset.
>
>> Signed-off-by: Tadeusz Struk<tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> v2:
>> * Use len local variable to avoid multiple calls to fdt32_to_cpu(*lenp)
>> * Add can_assume(VALID_DTB) to the new checks
>> libfdt/fdt.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
>> index 90a39e8..b7c202a 100644
>> --- a/libfdt/fdt.c
>> +++ b/libfdt/fdt.c
>> @@ -162,7 +162,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>> uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>> {
>> const fdt32_t *tagp, *lenp;
>> - uint32_t tag;
>> + uint32_t tag, len;
>> int offset = startoffset;
>> const char *p;
>>
>> @@ -188,12 +188,20 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>> lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
>> if (!can_assume(VALID_DTB) && !lenp)
>> return FDT_END; /* premature end */
>> +
>> + len = fdt32_to_cpu(*lenp);
>> + if (!can_assume(VALID_DTB) && INT_MAX <= len)
> This check is redundant with the one below, isn't it?
>
>> + return FDT_END; /* premature end */
>> +
>> /* skip-name offset, length and value */
>> - offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>> - + fdt32_to_cpu(*lenp);
>> + offset += sizeof(struct fdt_property) - FDT_TAGSIZE + len;
>> +
>> + if (!can_assume(VALID_DTB) && offset < 0)
>> + return FDT_END; /* premature end */
> Hmmm.. so, signed integer overflow is actually undefined behaviour in
> C, so checking for offset < 0 after the addition isn't actually a safe
> way to check for overflow. To safely check for overflow, I believe
> you need to check that the*unsigned* sum of offset and len is greater
> than or equal to offset (*unsigned* integer overflow is defined to
> wrap as you'd expect for 2s complement arithmetic). Actually given
> the constraints we've put on offsets in general, we need to check that
> that unsigned sum is both greater than offset and less than INT_MAX.
Thanks for feedback. I will change the logic to only work on unsigned
integers. I will also update the unit tests according to your suggestions.
--
Thanks,
Tadeusz
prev parent reply other threads:[~2022-10-04 23:06 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-30 15:20 [PATCH v2 1/2] libfdt: prevent integer overflow in fdt_next_tag Tadeusz Struk
[not found] ` <20220930152004.674591-1-tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2022-09-30 15:20 ` [PATCH v2 2/2] libfdt: tests: add get_next_tag_invalid_prop_len Tadeusz Struk
[not found] ` <20220930152004.674591-2-tadeusz.struk-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2022-10-04 7:15 ` David Gibson
2022-10-04 7:06 ` [PATCH v2 1/2] libfdt: prevent integer overflow in fdt_next_tag David Gibson
2022-10-04 23:06 ` Tadeusz Struk [this message]
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=53f2985d-15a8-d266-2fbe-ed557036ad6b@linaro.org \
--to=tadeusz.struk-qsej5fyqhm4dnm+yrofe0a@public.gmane.org \
--cc=david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org \
--cc=devicetree-compiler-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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 read-only IMAP folder(s) and NNTP newsgroup(s).