* [PATCH 0/1] lib/vsprintf.c: fix integer overflow in vsprintf @ 2023-03-09 2:12 Ying-Chun Liu (PaulLiu) 2023-03-09 2:12 ` [PATCH 1/1] " Ying-Chun Liu (PaulLiu) 0 siblings, 1 reply; 6+ messages in thread From: Ying-Chun Liu (PaulLiu) @ 2023-03-09 2:12 UTC (permalink / raw) To: u-boot; +Cc: 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf 2023-03-09 2:12 [PATCH 0/1] lib/vsprintf.c: fix integer overflow in vsprintf Ying-Chun Liu (PaulLiu) @ 2023-03-09 2:12 ` Ying-Chun Liu (PaulLiu) 2023-03-09 9:45 ` Rasmus Villemoes 2023-08-15 14:42 ` Tom Rini 0 siblings, 2 replies; 6+ messages in thread From: Ying-Chun Liu (PaulLiu) @ 2023-03-09 2:12 UTC (permalink / raw) To: u-boot; +Cc: Tom Cherry, Ying-Chun Liu, Tom Rini 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf 2023-03-09 2:12 ` [PATCH 1/1] " Ying-Chun Liu (PaulLiu) @ 2023-03-09 9:45 ` Rasmus Villemoes 2023-08-15 14:42 ` Tom Rini 1 sibling, 0 replies; 6+ messages in thread From: Rasmus Villemoes @ 2023-03-09 9:45 UTC (permalink / raw) To: Ying-Chun Liu (PaulLiu), u-boot; +Cc: Tom Rini 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf 2023-03-09 2:12 ` [PATCH 1/1] " Ying-Chun Liu (PaulLiu) 2023-03-09 9:45 ` Rasmus Villemoes @ 2023-08-15 14:42 ` Tom Rini 2023-08-15 15:33 ` Paul Liu 1 sibling, 1 reply; 6+ messages in thread From: Tom Rini @ 2023-08-15 14:42 UTC (permalink / raw) To: Ying-Chun Liu (PaulLiu); +Cc: u-boot, Tom Cherry [-- 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf 2023-08-15 14:42 ` Tom Rini @ 2023-08-15 15:33 ` Paul Liu 2023-08-17 23:49 ` Tom Cherry 0 siblings, 1 reply; 6+ messages in thread From: Paul Liu @ 2023-08-15 15:33 UTC (permalink / raw) To: Tom Rini; +Cc: u-boot, Tom Cherry 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] lib/vsprintf.c: fix integer overflow in vsprintf 2023-08-15 15:33 ` Paul Liu @ 2023-08-17 23:49 ` Tom Cherry 0 siblings, 0 replies; 6+ messages in thread From: Tom Cherry @ 2023-08-17 23:49 UTC (permalink / raw) To: Paul Liu, Alistair Delva; +Cc: Tom Rini, u-boot 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-18 14:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-03-09 2:12 [PATCH 0/1] lib/vsprintf.c: fix integer overflow in vsprintf Ying-Chun Liu (PaulLiu) 2023-03-09 2:12 ` [PATCH 1/1] " Ying-Chun Liu (PaulLiu) 2023-03-09 9:45 ` Rasmus Villemoes 2023-08-15 14:42 ` Tom Rini 2023-08-15 15:33 ` Paul Liu 2023-08-17 23:49 ` Tom Cherry
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox