| Message ID | 20230309021221.306044-2-paul.liu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | lib/vsprintf.c: fix integer overflow in vsprintf | expand |
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/+/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.
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 >
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
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, ...)