[PATCH 0/1] lib/vsprintf.c: fix integer overflow in vsprintf
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(-)
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/+/43aae5d4415e0f9d... --- 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, ...)
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
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/+/43aae5d4415e0f9d...
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.
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/+/43aae5d4415e0f9d...
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
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/+/43aae5d4415e0f9d...
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
participants (5)
-
Paul Liu -
Rasmus Villemoes -
Tom Cherry -
Tom Rini -
Ying-Chun Liu (PaulLiu)