| Message ID | 20230512021033.1378730-6-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix qemu_strtosz() read-out-of-bounds | expand |
On Thu, May 11, 2023 at 09:10:19PM -0500, Eric Blake wrote: > > While we were matching 32-bit strtol in qemu_strtoi, our use of a > 64-bit parse was leaking through for some inaccurate answers in > qemu_strtoui in comparison to a 32-bit strtoul. Fix those, and update > the testsuite now that our bounds checks are correct. > > Our int wrappers would be a lot easier to write if libc had a > guaranteed 32-bit parser even on platforms with 64-bit long. > > Fixes: 473a2a331e ("cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types", v2.12.0) > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/unit/test-cutils.c | 11 +++++------ > util/cutils.c | 14 ++++++++++---- > 2 files changed, 15 insertions(+), 10 deletions(-) cc'ing qemu-stable as this is a bug fix, but given its age, it's not a recent regression and therefore probably not essential for backport
On 12.05.23 04:10, Eric Blake wrote: > While we were matching 32-bit strtol in qemu_strtoi, our use of a > 64-bit parse was leaking through for some inaccurate answers in > qemu_strtoui in comparison to a 32-bit strtoul. Fix those, and update > the testsuite now that our bounds checks are correct. > > Our int wrappers would be a lot easier to write if libc had a > guaranteed 32-bit parser even on platforms with 64-bit long. > > Fixes: 473a2a331e ("cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types", v2.12.0) > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > tests/unit/test-cutils.c | 11 +++++------ > util/cutils.c | 14 ++++++++++---- > 2 files changed, 15 insertions(+), 10 deletions(-) Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > diff --git a/util/cutils.c b/util/cutils.c > index 5887e744140..997ddcd09e5 100644 > --- a/util/cutils.c > +++ b/util/cutils.c > @@ -466,10 +466,16 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base, > if (errno == ERANGE) { > *result = -1; > } else { > - if (lresult > UINT_MAX) { > - *result = UINT_MAX; > - errno = ERANGE; > - } else if (lresult < INT_MIN) { > + /* > + * Note that platforms with 32-bit strtoul accept input in the > + * range [-4294967295, 4294967295]; but we used 64-bit > + * strtoull which wraps -18446744073709551615 to 1. Reject > + * positive values that contain '-', and wrap all valid > + * negative values. > + */ > + if (lresult > UINT_MAX || > + lresult < -(long long)UINT_MAX || > + (lresult > 0 && memchr(nptr, '-', ep - nptr))) { > *result = UINT_MAX; > errno = ERANGE; > } else { Just a question whether I guessed correctly, because there’s no comment on the matter: We store the (supposedly unsigned) result of strtoull() in a signed long long because e.g. -1 is mapped to ULLONG_MAX, so the valid unsigned ranges would be [0, UINT_MAX] \cup [ULLONG_MAX - UINT_MAX + 1, ULLONG_MAX], which is more cumbersome to check than the [-UINT_MAX, UINT_MAX] range? (And we’d need to exclude strings with - in them if ullresult > UINT_MAX rather than > 0, probably) Hanna
On Fri, May 19, 2023 at 04:42:11PM +0200, Hanna Czenczek wrote: > On 12.05.23 04:10, Eric Blake wrote: > > While we were matching 32-bit strtol in qemu_strtoi, our use of a > > 64-bit parse was leaking through for some inaccurate answers in > > qemu_strtoui in comparison to a 32-bit strtoul. Fix those, and update > > the testsuite now that our bounds checks are correct. > > > > Our int wrappers would be a lot easier to write if libc had a > > guaranteed 32-bit parser even on platforms with 64-bit long. > > > > Fixes: 473a2a331e ("cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types", v2.12.0) > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > tests/unit/test-cutils.c | 11 +++++------ > > util/cutils.c | 14 ++++++++++---- > > 2 files changed, 15 insertions(+), 10 deletions(-) > > Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > > > diff --git a/util/cutils.c b/util/cutils.c > > index 5887e744140..997ddcd09e5 100644 > > --- a/util/cutils.c > > +++ b/util/cutils.c > > @@ -466,10 +466,16 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base, Adding context: long long lresult; ... lresult = strtoull(nptr, &ep, base); > > if (errno == ERANGE) { > > *result = -1; > > } else { > > - if (lresult > UINT_MAX) { > > - *result = UINT_MAX; > > - errno = ERANGE; > > - } else if (lresult < INT_MIN) { > > + /* > > + * Note that platforms with 32-bit strtoul accept input in the > > + * range [-4294967295, 4294967295]; but we used 64-bit > > + * strtoull which wraps -18446744073709551615 to 1. Reject > > + * positive values that contain '-', and wrap all valid > > + * negative values. > > + */ > > + if (lresult > UINT_MAX || > > + lresult < -(long long)UINT_MAX || > > + (lresult > 0 && memchr(nptr, '-', ep - nptr))) { > > *result = UINT_MAX; > > errno = ERANGE; > > } else { > > Just a question whether I guessed correctly, because there’s no comment on > the matter: We store the (supposedly unsigned) result of strtoull() in a > signed long long because e.g. -1 is mapped to ULLONG_MAX, so the valid > unsigned ranges would be [0, UINT_MAX] \cup [ULLONG_MAX - UINT_MAX + 1, > ULLONG_MAX], which is more cumbersome to check than the [-UINT_MAX, > UINT_MAX] range? (And we’d need to exclude strings with - in them if > ullresult > UINT_MAX rather than > 0, probably) Yes. Not only will I take that as a hint that I should improve my commit message, but you have pointed out a hole in my unit testing and which is not fixed here; namely, strings like "-0xffffffffffff0000" are accepted when they should be rejected. I'm thinking the commit message should be something along the lines of the following: Note that strtoull takes two input ranges that overlap into a single output range: [-0xffffffffffffffff, -1] which maps to [1, 0xffffffffffffffff], and [0, 0xffffffffffffffff] maps to itself. The output value alone does not tell us whether the input was in the desired range [-0xffffffff, 0xffffffff], or in one of the other two ranges that also map to the desired range [-0xffffffffffffffff, -0xffffffff00000001] and [0xffffffff00000001, 0xffffffffffffffff], so we need to do some additional filtering. Merely checking whether strlen(input) is longer than strlen("-4294967295") is not going to work, because ("000000000000000000000000000") is longer but in range; but we can check for the presence or absence of '-' in the input being inconsistent with the resulting sign when the number is cast as a signed long long. v3 of this series will come soon, once you finish finding anything else in v2 I need to fix.
diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c index 89c10f5307a..08989d1d3ac 100644 --- a/tests/unit/test-cutils.c +++ b/tests/unit/test-cutils.c @@ -858,7 +858,7 @@ static void test_qemu_strtoui_hex(void) static void test_qemu_strtoui_wrap(void) { - /* FIXME - wraparound should be consistent with 32-bit strtoul */ + /* wraparound is consistent with 32-bit strtoul */ const char *str = "-4294967295"; /* 1 mod 2^32 */ char f = 'X'; const char *endptr = &f; @@ -867,8 +867,8 @@ static void test_qemu_strtoui_wrap(void) err = qemu_strtoui(str, &endptr, 0, &res); - g_assert_cmpint(err, ==, -ERANGE /* FIXME 0 */); - g_assert_cmphex(res, ==, UINT_MAX /* FIXME 1 */); + g_assert_cmpint(err, ==, 0); + g_assert_cmphex(res, ==, 1); g_assert_true(endptr == str + strlen(str)); } @@ -935,13 +935,12 @@ static void test_qemu_strtoui_underflow(void) g_assert_cmpint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); - /* FIXME - overflow should be consistent with 32-bit strtoul */ str = "-18446744073709551615"; /* -UINT64_MAX (not 1) */ endptr = "somewhere"; res = 999; err = qemu_strtoui(str, &endptr, 0, &res); - g_assert_cmpint(err, ==, 0 /* FIXME -ERANGE */); - g_assert_cmpint(res, ==, 1 /* FIXME UINT_MAX */); + g_assert_cmpint(err, ==, -ERANGE); + g_assert_cmpint(res, ==, UINT_MAX); g_assert_true(endptr == str + strlen(str)); str = "-0x10000000000000000"; /* 65 bits, 32-bit sign bit clear */ diff --git a/util/cutils.c b/util/cutils.c index 5887e744140..997ddcd09e5 100644 --- a/util/cutils.c +++ b/util/cutils.c @@ -466,10 +466,16 @@ int qemu_strtoui(const char *nptr, const char **endptr, int base, if (errno == ERANGE) { *result = -1; } else { - if (lresult > UINT_MAX) { - *result = UINT_MAX; - errno = ERANGE; - } else if (lresult < INT_MIN) { + /* + * Note that platforms with 32-bit strtoul accept input in the + * range [-4294967295, 4294967295]; but we used 64-bit + * strtoull which wraps -18446744073709551615 to 1. Reject + * positive values that contain '-', and wrap all valid + * negative values. + */ + if (lresult > UINT_MAX || + lresult < -(long long)UINT_MAX || + (lresult > 0 && memchr(nptr, '-', ep - nptr))) { *result = UINT_MAX; errno = ERANGE; } else {
While we were matching 32-bit strtol in qemu_strtoi, our use of a 64-bit parse was leaking through for some inaccurate answers in qemu_strtoui in comparison to a 32-bit strtoul. Fix those, and update the testsuite now that our bounds checks are correct. Our int wrappers would be a lot easier to write if libc had a guaranteed 32-bit parser even on platforms with 64-bit long. Fixes: 473a2a331e ("cutils: add qemu_strtoi & qemu_strtoui parsers for int/unsigned int types", v2.12.0) Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/unit/test-cutils.c | 11 +++++------ util/cutils.c | 14 ++++++++++---- 2 files changed, 15 insertions(+), 10 deletions(-)