[Qemu-devel] [PATCH] qdev: Validate hex properties

[Qemu-devel] [PATCH] qdev: Validate hex properties

[Hannes Reinecke]

strtoull() might return '-1' to signify an overflow.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 hw/core/qdev-properties.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index dc8ae69..5761295 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -202,6 +202,9 @@ static int parse_hex8(DeviceState *dev, Property *prop, const char *str)
     if ((*end != '\0') || (end == str)) {
         return -EINVAL;
     }
+    if (*ptr == (uint8_t)-1) {
+        return -ERANGE;
+    }
 
     return 0;
 }
@@ -333,6 +336,9 @@ static int parse_hex32(DeviceState *dev, Property *prop, const char *str)
     if ((*end != '\0') || (end == str)) {
         return -EINVAL;
     }
+    if (*ptr == (uint32_t)-1) {
+        return -ERANGE;
+    }
 
     return 0;
 }
@@ -400,6 +406,9 @@ static int parse_hex64(DeviceState *dev, Property *prop, const char *str)
     if ((*end != '\0') || (end == str)) {
         return -EINVAL;
     }
+    if (*ptr == (uint64_t)-1) {
+        return -ERANGE;
+    }
 
     return 0;
 }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] qdev: Validate hex properties

[Peter Maydell]

On 26 November 2013 15:05, Hannes Reinecke <hare@suse.de> wrote:
> strtoull() might return '-1' to signify an overflow.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  hw/core/qdev-properties.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index dc8ae69..5761295 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -202,6 +202,9 @@ static int parse_hex8(DeviceState *dev, Property *prop, const char *str)
>      if ((*end != '\0') || (end == str)) {
>          return -EINVAL;
>      }
> +    if (*ptr == (uint8_t)-1) {
> +        return -ERANGE;
> +    }

This doesn't look right -- *ptr might legitimately be anything
from 0x00 to 0xff; -1 can be a valid strtoul() return as well as
the error indication.

The right way to check for overflow is to clear errno before the call
to strtoul(), and then check whether it's non-zero after the call;
this is the approach recommended by POSIX.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtoul.html

For the hex8 version where we're putting the result in a smaller
sized integer than the type returned by strtoul(), we should
put the result into an unsigned long, check it's in 0..255, and
then stick it in the uint8_t.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] qdev: Validate hex properties

[Eric Blake]

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

On 11/26/2013 08:05 AM, Hannes Reinecke wrote:
> strtoull() might return '-1' to signify an overflow.

Yes, but -1 (aka ULLONG_MAX) is also a valid return when there is not
overflow, so testing for -1 is NOT a reliable indicator on whether
overflow occurred.  Also, you mention strtoull() here...

> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  hw/core/qdev-properties.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
> index dc8ae69..5761295 100644
> --- a/hw/core/qdev-properties.c
> +++ b/hw/core/qdev-properties.c
> @@ -202,6 +202,9 @@ static int parse_hex8(DeviceState *dev, Property *prop, const char *str)
>      if ((*end != '\0') || (end == str)) {

...but this was calling strtoul().

>          return -EINVAL;
>      }
> +    if (*ptr == (uint8_t)-1) {
> +        return -ERANGE;
> +    }

NAK.  This is NOT how you check for overflow with strtoull.  Instead,
you should use:

errno = 0;
*ptr = strtoul(str, &end, 16);
if (errno) {
    return -errno;
}
if (!*end || end == str) {
    return -EINVAL;
}
return 0;

>  
>      return 0;
>  }
> @@ -333,6 +336,9 @@ static int parse_hex32(DeviceState *dev, Property *prop, const char *str)
>      if ((*end != '\0') || (end == str)) {
>          return -EINVAL;
>      }
> +    if (*ptr == (uint32_t)-1) {
> +        return -ERANGE;
> +    }

NAK.  And this function is even MORE broken.  On a 32-bit platform,
assigning the results of strtol() into a uint32_t may result in silent
truncation.  If you are trying to detect overflow, you MUST assign the
results into a long, then check that the long does not overflow
uint32_t, rather than checking (too late) whether the uint32_t has
already been overflowed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]