[PATCH 0/1] lib/vsprintf.c: fix integer overflow in vsprintf

[PATCH 0/1] lib/vsprintf.c: fix integer overflow in vsprintf

[Ying-Chun Liu (PaulLiu)]

vsnprintf_internal() adds 'size' to 'buf' and vsprintf() sets 'size'
to 'INT_MAX' which can overflow.  This causes sprintf() to fail when
initializing the environment on 8GB.

Instead of using 'INT_MAX', we use SIZE_MAX - buf, which is the
largest possible string that could fit without overflowing 'size'.

Tom Cherry (1):
  lib/vsprintf.c: fix integer overflow in vsprintf

 lib/vsprintf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.39.2

[PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf

[Ying-Chun Liu (PaulLiu)]

From: Tom Cherry <tomcherry@google.com>

vsnprintf_internal() adds 'size' to 'buf' and vsprintf() sets 'size'
to 'INT_MAX' which can overflow.  This causes sprintf() to fail when
initializing the environment on 8GB.

Instead of using 'INT_MAX', we use SIZE_MAX - buf, which is the
largest possible string that could fit without overflowing 'size'.

Signed-off-by: Tom Cherry <tomcherry@google.com>
[ Paul: pick from the Android tree. Rebase to the upstream ]
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Tom Rini <trini@konsulko.com>
Link: https://android.googlesource.com/platform/external/u-boot/+/43aae5d4415e0f9d744fb798acd52429d09957ce
---
 lib/vsprintf.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 2d13e68b57..cd89c56a8f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -794,7 +794,12 @@ int scnprintf(char *buf, size_t size, const char *fmt, ...)
  */
 int vsprintf(char *buf, const char *fmt, va_list args)
 {
-	return vsnprintf_internal(buf, INT_MAX, fmt, args);
+	/* vsnprintf_internal adds size to buf, so use a size that won't
+	 * overflow.
+	 */
+	size_t max_size = SIZE_MAX - (size_t)buf;
+
+	return vsnprintf_internal(buf, max_size, fmt, args);
 }
 
 int sprintf(char *buf, const char *fmt, ...)
-- 
2.39.2

Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf

[Rasmus Villemoes]

On 09/03/2023 03.12, Ying-Chun Liu (PaulLiu) wrote:
> From: Tom Cherry <tomcherry@google.com>
> 
> vsnprintf_internal() adds 'size' to 'buf' and vsprintf() sets 'size'
> to 'INT_MAX' which can overflow. 

Yes, and? vsprintf_internal then detects that by looking at whether
"end" is now before "buf", and if so corrects it by setting end to the
largest possible address - which is more or less the same you do here,
except if for the platform in question sizeof(size_t)!=sizeof(void *).
So what exactly does this fix?

That piece of code is stolen from linux, so if it's a problem in U-Boot
it most definitely should also show up in linux, which it doesn't.

More details please. What platform is this, what is sizeof(size_t) and
sizeof(void *) and how does the amount of actual RAM come into the picture?

Rasmus

Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Thu, Mar 09, 2023 at 10:12:21AM +0800, Ying-Chun Liu (PaulLiu) wrote:

> From: Tom Cherry <tomcherry@google.com>
> 
> vsnprintf_internal() adds 'size' to 'buf' and vsprintf() sets 'size'
> to 'INT_MAX' which can overflow.  This causes sprintf() to fail when
> initializing the environment on 8GB.
> 
> Instead of using 'INT_MAX', we use SIZE_MAX - buf, which is the
> largest possible string that could fit without overflowing 'size'.
> 
> Signed-off-by: Tom Cherry <tomcherry@google.com>
> [ Paul: pick from the Android tree. Rebase to the upstream ]
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Tom Rini <trini@konsulko.com>
> Link: https://android.googlesource.com/platform/external/u-boot/+/43aae5d4415e0f9d744fb798acd52429d09957ce

So, this link here leads back to
https://issuetracker.google.com/issues/200479053 which isn't public.

Rasmus followed up and asked pointed questions, that weren't followed up
on.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf

[Paul Liu]

Hi Tom,

Yes, I think Rasmus is correct. I didn't have any real cases that can
trigger the bug.
So let's don't include this patch. I'll see if I can revert this in
AOSP's branch.

Yours,
Paul



Y

On Tue, 15 Aug 2023 at 22:42, Tom Rini <trini@konsulko.com> wrote:

> On Thu, Mar 09, 2023 at 10:12:21AM +0800, Ying-Chun Liu (PaulLiu) wrote:
>
> > From: Tom Cherry <tomcherry@google.com>
> >
> > vsnprintf_internal() adds 'size' to 'buf' and vsprintf() sets 'size'
> > to 'INT_MAX' which can overflow.  This causes sprintf() to fail when
> > initializing the environment on 8GB.
> >
> > Instead of using 'INT_MAX', we use SIZE_MAX - buf, which is the
> > largest possible string that could fit without overflowing 'size'.
> >
> > Signed-off-by: Tom Cherry <tomcherry@google.com>
> > [ Paul: pick from the Android tree. Rebase to the upstream ]
> > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> > Cc: Tom Rini <trini@konsulko.com>
> > Link:
> https://android.googlesource.com/platform/external/u-boot/+/43aae5d4415e0f9d744fb798acd52429d09957ce
>
> So, this link here leads back to
> https://issuetracker.google.com/issues/200479053 which isn't public.
>
> Rasmus followed up and asked pointed questions, that weren't followed up
> on.
>
> --
> Tom
>

Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf

[Tom Cherry]

On Tue, Aug 15, 2023 at 8:33 AM Paul Liu <paul.liu@linaro.org> wrote:
>
> Hi Tom,
>
> Yes, I think Rasmus is correct. I didn't have any real cases that can trigger the bug.
> So let's don't include this patch. I'll see if I can revert this in AOSP's branch.
>
> Yours,
> Paul
>
>
>
> Y
>
> On Tue, 15 Aug 2023 at 22:42, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Thu, Mar 09, 2023 at 10:12:21AM +0800, Ying-Chun Liu (PaulLiu) wrote:
>>
>> > From: Tom Cherry <tomcherry@google.com>
>> >
>> > vsnprintf_internal() adds 'size' to 'buf' and vsprintf() sets 'size'
>> > to 'INT_MAX' which can overflow.  This causes sprintf() to fail when
>> > initializing the environment on 8GB.
>> >
>> > Instead of using 'INT_MAX', we use SIZE_MAX - buf, which is the
>> > largest possible string that could fit without overflowing 'size'.
>> >
>> > Signed-off-by: Tom Cherry <tomcherry@google.com>
>> > [ Paul: pick from the Android tree. Rebase to the upstream ]
>> > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
>> > Cc: Tom Rini <trini@konsulko.com>
>> > Link: https://android.googlesource.com/platform/external/u-boot/+/43aae5d4415e0f9d744fb798acd52429d09957ce
>>
>> So, this link here leads back to
>> https://issuetracker.google.com/issues/200479053 which isn't public.
>>
>> Rasmus followed up and asked pointed questions, that weren't followed up
>> on.
>>
>> --
>> Tom

Hi all,

I hadn't seen the questions from Rasmus, and I haven't had much time
to dig into why this issue happened. I'll try to get time for this in
the upcoming weeks.

I originally triggered this bug in a real-world use case. I was unable
to boot my platform before this patch and I wrote this patch to solve
that issue. I reverted that patch and tried booting my platform today
and I am able to boot however I see error logs that are not present
when this patch is applied (the below "Partition 1: invalid GUID"),
which suggests that there are lingering issues, which I'll
investigate.

With the change:

U-Boot 2023.04-maybe-dirty (Jan 01 1970 - 00:00:00 +0000)

DRAM:  8 GiB
Core:  8 devices, 8 uclasses, devicetree: separate
Hit any key to stop autoboot:  0
ANDROID: Booting Unlocked!!
## Android Verified Boot 2.0 version 1.1.0

With the change reverted:

U-Boot 2023.04-maybe-dirty (Jan 01 1970 - 00:00:00 +0000)

DRAM:  8 GiB
Core:  8 devices, 8 uclasses, devicetree: separate
Hit any key to stop autoboot:  0
Partition 1: invalid GUID
Partition 2: invalid GUID
Partition 3: invalid GUID
Partition 4: invalid GUID
Partition 5: invalid GUID
ANDROID: Booting Unlocked!!
## Android Verified Boot 2.0 version 1.1.0

Thanks
Tom