[Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Ildar Isaev]

'offset' field in struct Property is calculated as a diff between two pointers (hw/core/qdev-properties.c:802)

arrayprop->prop.offset = eltptr - (void *)dev;

If offset is declared as int, this subtraction can cause type overflow
thus leading to the fall of the subsequent assert (hw/core/qdev-properties.c:803)

assert(qdev_get_prop_ptr(dev, &arrayprop->prop) == eltptr);

So ptrdiff_t should be used instead

Signed-off-by: Ildar Isaev <ild@inbox.ru>
---
 include/hw/qdev-core.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..f0e2a73 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -224,7 +224,7 @@ struct BusState {
 struct Property {
     const char   *name;
     PropertyInfo *info;
-    int          offset;
+    ptrdiff_t    offset;
     uint8_t      bitnr;
     uint8_t      qtype;
     int64_t      defval;
-- 
1.9.3

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Markus Armbruster]

Stumbled over this while throwing away old mail.  Andreas, what do you
think?

Ildar Isaev <ild@inbox.ru> writes:

> 'offset' field in struct Property is calculated as a diff between two pointers (hw/core/qdev-properties.c:802)
>
> arrayprop->prop.offset = eltptr - (void *)dev;
>
> If offset is declared as int, this subtraction can cause type overflow
> thus leading to the fall of the subsequent assert (hw/core/qdev-properties.c:803)
>
> assert(qdev_get_prop_ptr(dev, &arrayprop->prop) == eltptr);
>
> So ptrdiff_t should be used instead
>
> Signed-off-by: Ildar Isaev <ild@inbox.ru>
> ---
>  include/hw/qdev-core.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 4e673f9..f0e2a73 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -224,7 +224,7 @@ struct BusState {
>  struct Property {
>      const char   *name;
>      PropertyInfo *info;
> -    int          offset;
> +    ptrdiff_t    offset;
>      uint8_t      bitnr;
>      uint8_t      qtype;
>      int64_t      defval;

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Peter Maydell]

On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> wrote:
> Stumbled over this while throwing away old mail.  Andreas, what do you
> think?

Seems right to me -- I suspect the original properties code was
written with the assumption that the property field would be
inside the device struct (and so offsets are small). The array
properties code breaks that assumption by allocating a separate
lump of memory with the properties in it; so now there's no
guarantee that the two pointers being subtracted will be
within 4G of each other.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Arguably for consistency the 'arrayoffset' struct member should
also be a ptrdiff_t, though our current uses of it are such
that it'll always be within int range.

-- PMM

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> wrote:
>> Stumbled over this while throwing away old mail.  Andreas, what do you
>> think?
>
> Seems right to me -- I suspect the original properties code was
> written with the assumption that the property field would be
> inside the device struct (and so offsets are small). The array
> properties code breaks that assumption by allocating a separate
> lump of memory with the properties in it; so now there's no
> guarantee that the two pointers being subtracted will be
> within 4G of each other.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Arguably for consistency the 'arrayoffset' struct member should
> also be a ptrdiff_t, though our current uses of it are such
> that it'll always be within int range.

Andreas?

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Andreas Färber]

Am 11.11.2015 um 09:54 schrieb Markus Armbruster:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> wrote:
>>> Stumbled over this while throwing away old mail.  Andreas, what do you
>>> think?
>>
>> Seems right to me -- I suspect the original properties code was
>> written with the assumption that the property field would be
>> inside the device struct (and so offsets are small). The array
>> properties code breaks that assumption by allocating a separate
>> lump of memory with the properties in it; so now there's no
>> guarantee that the two pointers being subtracted will be
>> within 4G of each other.
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>
>> Arguably for consistency the 'arrayoffset' struct member should
>> also be a ptrdiff_t, though our current uses of it are such
>> that it'll always be within int range.
> 
> Andreas?

Found it archived. I honestly don't think it's necessary in practice to
have 64-bit offsets on 64-bit host, but it builds okay, queued. Testing
got stuck in ahci though, investigating.

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[John Snow]

On 11/12/2015 12:41 PM, Andreas Färber wrote:
> Am 11.11.2015 um 09:54 schrieb Markus Armbruster:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> On 25 August 2015 at 15:17, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Stumbled over this while throwing away old mail.  Andreas, what do you
>>>> think?
>>>
>>> Seems right to me -- I suspect the original properties code was
>>> written with the assumption that the property field would be
>>> inside the device struct (and so offsets are small). The array
>>> properties code breaks that assumption by allocating a separate
>>> lump of memory with the properties in it; so now there's no
>>> guarantee that the two pointers being subtracted will be
>>> within 4G of each other.
>>>
>>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>>>
>>> Arguably for consistency the 'arrayoffset' struct member should
>>> also be a ptrdiff_t, though our current uses of it are such
>>> that it'll always be within int range.
>>
>> Andreas?
> 
> Found it archived. I honestly don't think it's necessary in practice to
> have 64-bit offsets on 64-bit host, but it builds okay, queued. Testing
> got stuck in ahci though, investigating.
> 
> Thanks,
> Andreas
> 

Did you ever reproduce this, or does it seem to just be a race?

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[Andreas Färber]

Am 13.11.2015 um 19:32 schrieb John Snow:
> On 11/12/2015 12:41 PM, Andreas Färber wrote:
>> [...] Testing
>> got stuck in ahci though, investigating.
>>
>> Thanks,
>> Andreas
>>
> 
> Did you ever reproduce this, or does it seem to just be a race?

Once I updated to a later git commit I was no longer able to reproduce
that hang to date. So it could either be a hard-to-reproduce race, or
some merge yesterday made it go away.

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH] include/hw: field 'offset' in struct Property should be ptrdiff_t as int causes overflow

[John Snow]

On 11/13/2015 01:36 PM, Andreas Färber wrote:
> Am 13.11.2015 um 19:32 schrieb John Snow:
>> On 11/12/2015 12:41 PM, Andreas Färber wrote:
>>> [...] Testing
>>> got stuck in ahci though, investigating.
>>>
>>> Thanks,
>>> Andreas
>>>
>>
>> Did you ever reproduce this, or does it seem to just be a race?
> 
> Once I updated to a later git commit I was no longer able to reproduce
> that hang to date. So it could either be a hard-to-reproduce race, or
> some merge yesterday made it go away.
> 
> Cheers,
> Andreas
> 

FWIW, on Fedora 22, I can't get my 64 or 32 bit build of the i386 target
to reproduce the behavior after a couple dozen runs.

I was using the same commit you pointed to, so it seems like a race is
likely. I'll keep my eye open.

--js