[Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
computation of @dt_size overflows to a negative number, which then
gets converted to a very large size_t for g_malloc0() and
load_image_size().  In the (fortunately improbable) case g_malloc0()
succeeds and load_image_size() survives, we'd assign the negative
number to *sizep.  What that would do to the callers I can't say, but
it's unlikely to be good.

Fix by rejecting images whose size would overflow.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 device_tree.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index 296278e12a..f8b46b3c73 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
                      filename_path);
         goto fail;
     }
+    if (dt_size > INT_MAX / 2 - 10000) {
+        error_report("Device tree file '%s' is too large", filename_path);
+        goto fail;
+    }
 
     /* Expand to 2x size to give enough room for manipulation.  */
     dt_size += 10000;
-- 
2.17.2

[Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
computation of @dt_size overflows to a negative number, which then
gets converted to a very large size_t for g_malloc0() and
load_image_size().  In the (fortunately improbable) case g_malloc0()
succeeds and load_image_size() survives, we'd assign the negative
number to *sizep.  What that would do to the callers I can't say, but
it's unlikely to be good.

Fix by rejecting images whose size would overflow.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 device_tree.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/device_tree.c b/device_tree.c
index 296278e12a..f8b46b3c73 100644
--- a/device_tree.c
+++ b/device_tree.c
@@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
                      filename_path);
         goto fail;
     }
+    if (dt_size > INT_MAX / 2 - 10000) {
+        error_report("Device tree file '%s' is too large", filename_path);
+        goto fail;
+    }
 
     /* Expand to 2x size to give enough room for manipulation.  */
     dt_size += 10000;
-- 
2.17.2

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Philippe Mathieu-Daudé]

On 4/9/19 7:40 PM, Markus Armbruster wrote:
> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> computation of @dt_size overflows to a negative number, which then
> gets converted to a very large size_t for g_malloc0() and
> load_image_size().  In the (fortunately improbable) case g_malloc0()
> succeeds and load_image_size() survives, we'd assign the negative
> number to *sizep.  What that would do to the callers I can't say, but
> it's unlikely to be good.
> 
> Fix by rejecting images whose size would overflow.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  device_tree.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/device_tree.c b/device_tree.c
> index 296278e12a..f8b46b3c73 100644
> --- a/device_tree.c
> +++ b/device_tree.c
> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>                       filename_path);
>          goto fail;
>      }
> +    if (dt_size > INT_MAX / 2 - 10000) {

We should avoid magic number duplication.
That said, this patch looks safe.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

BTW how did you figure that out?

> +        error_report("Device tree file '%s' is too large", filename_path);
> +        goto fail;
> +    }
>  
>      /* Expand to 2x size to give enough room for manipulation.  */
>      dt_size += 10000;
> 

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Peter Maydell]

On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
>
> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> computation of @dt_size overflows to a negative number, which then
> gets converted to a very large size_t for g_malloc0() and
> load_image_size().  In the (fortunately improbable) case g_malloc0()
> succeeds and load_image_size() survives, we'd assign the negative
> number to *sizep.  What that would do to the callers I can't say, but
> it's unlikely to be good.
>
> Fix by rejecting images whose size would overflow.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

I think this patch is missing some attributions for the
security researchers who found the issue initially.
PJP's patch for this from a couple of weeks back has a
reported-by credit:
https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Peter Maydell]

On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
>
> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> computation of @dt_size overflows to a negative number, which then
> gets converted to a very large size_t for g_malloc0() and
> load_image_size().  In the (fortunately improbable) case g_malloc0()
> succeeds and load_image_size() survives, we'd assign the negative
> number to *sizep.  What that would do to the callers I can't say, but
> it's unlikely to be good.
>
> Fix by rejecting images whose size would overflow.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

I think this patch is missing some attributions for the
security researchers who found the issue initially.
PJP's patch for this from a couple of weeks back has a
reported-by credit:
https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Alistair Francis]

On Tue, Apr 9, 2019 at 1:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> > computation of @dt_size overflows to a negative number, which then
> > gets converted to a very large size_t for g_malloc0() and
> > load_image_size().  In the (fortunately improbable) case g_malloc0()
> > succeeds and load_image_size() survives, we'd assign the negative
> > number to *sizep.  What that would do to the callers I can't say, but
> > it's unlikely to be good.
> >
> > Fix by rejecting images whose size would overflow.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> I think this patch is missing some attributions for the
> security researchers who found the issue initially.
> PJP's patch for this from a couple of weeks back has a
> reported-by credit:
> https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

It seems like from that discussion that this patch is the correct approach.

I can add the attributions and send a PR for 4.0. I'll send it by EOD
unless anyone has any objections.

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Alistair Francis]

On Tue, Apr 9, 2019 at 1:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
> >
> > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> > computation of @dt_size overflows to a negative number, which then
> > gets converted to a very large size_t for g_malloc0() and
> > load_image_size().  In the (fortunately improbable) case g_malloc0()
> > succeeds and load_image_size() survives, we'd assign the negative
> > number to *sizep.  What that would do to the callers I can't say, but
> > it's unlikely to be good.
> >
> > Fix by rejecting images whose size would overflow.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> I think this patch is missing some attributions for the
> security researchers who found the issue initially.
> PJP's patch for this from a couple of weeks back has a
> reported-by credit:
> https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

It seems like from that discussion that this patch is the correct approach.

I can add the attributions and send a PR for 4.0. I'll send it by EOD
unless anyone has any objections.

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Peter Maydell]

On Tue, 9 Apr 2019 at 21:15, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Apr 9, 2019 at 1:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> > > computation of @dt_size overflows to a negative number, which then
> > > gets converted to a very large size_t for g_malloc0() and
> > > load_image_size().  In the (fortunately improbable) case g_malloc0()
> > > succeeds and load_image_size() survives, we'd assign the negative
> > > number to *sizep.  What that would do to the callers I can't say, but
> > > it's unlikely to be good.
> > >
> > > Fix by rejecting images whose size would overflow.
> > >
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > I think this patch is missing some attributions for the
> > security researchers who found the issue initially.
> > PJP's patch for this from a couple of weeks back has a
> > reported-by credit:
> > https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/
>
> It seems like from that discussion that this patch is the correct approach.
>
> I can add the attributions and send a PR for 4.0. I'll send it by EOD
> unless anyone has any objections.

Thanks. I think given it's 21:30 here I'm going to postpone
tagging rc3 til tomorrow (mid-afternoon UK time). I'm still
hoping we can avoid an rc4...

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Peter Maydell]

On Tue, 9 Apr 2019 at 21:15, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Apr 9, 2019 at 1:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
> > >
> > > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> > > computation of @dt_size overflows to a negative number, which then
> > > gets converted to a very large size_t for g_malloc0() and
> > > load_image_size().  In the (fortunately improbable) case g_malloc0()
> > > succeeds and load_image_size() survives, we'd assign the negative
> > > number to *sizep.  What that would do to the callers I can't say, but
> > > it's unlikely to be good.
> > >
> > > Fix by rejecting images whose size would overflow.
> > >
> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >
> > I think this patch is missing some attributions for the
> > security researchers who found the issue initially.
> > PJP's patch for this from a couple of weeks back has a
> > reported-by credit:
> > https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/
>
> It seems like from that discussion that this patch is the correct approach.
>
> I can add the attributions and send a PR for 4.0. I'll send it by EOD
> unless anyone has any objections.

Thanks. I think given it's 21:30 here I'm going to postpone
tagging rc3 til tomorrow (mid-afternoon UK time). I'm still
hoping we can avoid an rc4...

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[David Gibson]

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

On Tue, Apr 09, 2019 at 08:59:55PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/9/19 7:40 PM, Markus Armbruster wrote:
> > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> > computation of @dt_size overflows to a negative number, which then
> > gets converted to a very large size_t for g_malloc0() and
> > load_image_size().  In the (fortunately improbable) case g_malloc0()
> > succeeds and load_image_size() survives, we'd assign the negative
> > number to *sizep.  What that would do to the callers I can't say, but
> > it's unlikely to be good.
> > 
> > Fix by rejecting images whose size would overflow.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  device_tree.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/device_tree.c b/device_tree.c
> > index 296278e12a..f8b46b3c73 100644
> > --- a/device_tree.c
> > +++ b/device_tree.c
> > @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
> >                       filename_path);
> >          goto fail;
> >      }
> > +    if (dt_size > INT_MAX / 2 - 10000) {
> 
> We should avoid magic number duplication.
> That said, this patch looks safe.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

As Philippe says, the test condition is kinda ugly and I hope we can
refine it in future.  But since it fixes a real problem for now,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[David Gibson]

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

On Tue, Apr 09, 2019 at 08:59:55PM +0200, Philippe Mathieu-Daudé wrote:
> On 4/9/19 7:40 PM, Markus Armbruster wrote:
> > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> > computation of @dt_size overflows to a negative number, which then
> > gets converted to a very large size_t for g_malloc0() and
> > load_image_size().  In the (fortunately improbable) case g_malloc0()
> > succeeds and load_image_size() survives, we'd assign the negative
> > number to *sizep.  What that would do to the callers I can't say, but
> > it's unlikely to be good.
> > 
> > Fix by rejecting images whose size would overflow.
> > 
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  device_tree.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/device_tree.c b/device_tree.c
> > index 296278e12a..f8b46b3c73 100644
> > --- a/device_tree.c
> > +++ b/device_tree.c
> > @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
> >                       filename_path);
> >          goto fail;
> >      }
> > +    if (dt_size > INT_MAX / 2 - 10000) {
> 
> We should avoid magic number duplication.
> That said, this patch looks safe.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

As Philippe says, the test condition is kinda ugly and I hope we can
refine it in future.  But since it fixes a real problem for now,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Philippe Mathieu-Daudé]

On 4/9/19 10:08 PM, Peter Maydell wrote:
> On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>> computation of @dt_size overflows to a negative number, which then
>> gets converted to a very large size_t for g_malloc0() and
>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>> succeeds and load_image_size() survives, we'd assign the negative
>> number to *sizep.  What that would do to the callers I can't say, but
>> it's unlikely to be good.
>>
>> Fix by rejecting images whose size would overflow.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> I think this patch is missing some attributions for the
> security researchers who found the issue initially.
> PJP's patch for this from a couple of weeks back has a
> reported-by credit:
> https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

Oh I missed that thread while on PTO. This answers my "how did you
figure that out?" question :)

Thanks,

Phil.

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/9/19 7:40 PM, Markus Armbruster wrote:
>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>> computation of @dt_size overflows to a negative number, which then
>> gets converted to a very large size_t for g_malloc0() and
>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>> succeeds and load_image_size() survives, we'd assign the negative
>> number to *sizep.  What that would do to the callers I can't say, but
>> it's unlikely to be good.
>> 
>> Fix by rejecting images whose size would overflow.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  device_tree.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/device_tree.c b/device_tree.c
>> index 296278e12a..f8b46b3c73 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>                       filename_path);
>>          goto fail;
>>      }
>> +    if (dt_size > INT_MAX / 2 - 10000) {
>
> We should avoid magic number duplication.
> That said, this patch looks safe.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

> BTW how did you figure that out?

Downstream handling of upstream commit da885fe1ee8 led me to the
function.  I spotted dt_size = get_image_size(filename_path).
Experience has taught me to check the left hand side's type.  Bad.  Then
I saw how dt_size gets increased.  Worse.

>> +        error_report("Device tree file '%s' is too large", filename_path);
>> +        goto fail;
>> +    }
>>  
>>      /* Expand to 2x size to give enough room for manipulation.  */
>>      dt_size += 10000;
>> 

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/9/19 7:40 PM, Markus Armbruster wrote:
>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>> computation of @dt_size overflows to a negative number, which then
>> gets converted to a very large size_t for g_malloc0() and
>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>> succeeds and load_image_size() survives, we'd assign the negative
>> number to *sizep.  What that would do to the callers I can't say, but
>> it's unlikely to be good.
>> 
>> Fix by rejecting images whose size would overflow.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  device_tree.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/device_tree.c b/device_tree.c
>> index 296278e12a..f8b46b3c73 100644
>> --- a/device_tree.c
>> +++ b/device_tree.c
>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>                       filename_path);
>>          goto fail;
>>      }
>> +    if (dt_size > INT_MAX / 2 - 10000) {
>
> We should avoid magic number duplication.
> That said, this patch looks safe.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

> BTW how did you figure that out?

Downstream handling of upstream commit da885fe1ee8 led me to the
function.  I spotted dt_size = get_image_size(filename_path).
Experience has taught me to check the left hand side's type.  Bad.  Then
I saw how dt_size gets increased.  Worse.

>> +        error_report("Device tree file '%s' is too large", filename_path);
>> +        goto fail;
>> +    }
>>  
>>      /* Expand to 2x size to give enough room for manipulation.  */
>>      dt_size += 10000;
>> 

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

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

> On Tue, 9 Apr 2019 at 21:15, Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Apr 9, 2019 at 1:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> > On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
>> > >
>> > > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>> > > computation of @dt_size overflows to a negative number, which then
>> > > gets converted to a very large size_t for g_malloc0() and
>> > > load_image_size().  In the (fortunately improbable) case g_malloc0()
>> > > succeeds and load_image_size() survives, we'd assign the negative
>> > > number to *sizep.  What that would do to the callers I can't say, but
>> > > it's unlikely to be good.
>> > >
>> > > Fix by rejecting images whose size would overflow.
>> > >
>> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > I think this patch is missing some attributions for the
>> > security researchers who found the issue initially.
>> > PJP's patch for this from a couple of weeks back has a
>> > reported-by credit:
>> > https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

Uh, I missed that thread.  Thanks for doing my homework for me!

>> It seems like from that discussion that this patch is the correct approach.
>>
>> I can add the attributions and send a PR for 4.0. I'll send it by EOD
>> unless anyone has any objections.
>
> Thanks. I think given it's 21:30 here I'm going to postpone
> tagging rc3 til tomorrow (mid-afternoon UK time). I'm still
> hoping we can avoid an rc4...

Want me to look for a few more integer overflows today?  ;-P

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

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

> On Tue, 9 Apr 2019 at 21:15, Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Apr 9, 2019 at 1:08 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>> >
>> > On Wed, 10 Apr 2019 at 00:40, Markus Armbruster <armbru@redhat.com> wrote:
>> > >
>> > > If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>> > > computation of @dt_size overflows to a negative number, which then
>> > > gets converted to a very large size_t for g_malloc0() and
>> > > load_image_size().  In the (fortunately improbable) case g_malloc0()
>> > > succeeds and load_image_size() survives, we'd assign the negative
>> > > number to *sizep.  What that would do to the callers I can't say, but
>> > > it's unlikely to be good.
>> > >
>> > > Fix by rejecting images whose size would overflow.
>> > >
>> > > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >
>> > I think this patch is missing some attributions for the
>> > security researchers who found the issue initially.
>> > PJP's patch for this from a couple of weeks back has a
>> > reported-by credit:
>> > https://patchew.org/QEMU/20190322073555.20889-1-ppandit@redhat.com/

Uh, I missed that thread.  Thanks for doing my homework for me!

>> It seems like from that discussion that this patch is the correct approach.
>>
>> I can add the attributions and send a PR for 4.0. I'll send it by EOD
>> unless anyone has any objections.
>
> Thanks. I think given it's 21:30 here I'm going to postpone
> tagging rc3 til tomorrow (mid-afternoon UK time). I'm still
> hoping we can avoid an rc4...

Want me to look for a few more integer overflows today?  ;-P

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Philippe Mathieu-Daudé]

On 4/10/19 7:28 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> On 4/9/19 7:40 PM, Markus Armbruster wrote:
>>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>>> computation of @dt_size overflows to a negative number, which then
>>> gets converted to a very large size_t for g_malloc0() and
>>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>>> succeeds and load_image_size() survives, we'd assign the negative
>>> number to *sizep.  What that would do to the callers I can't say, but
>>> it's unlikely to be good.
>>>
>>> Fix by rejecting images whose size would overflow.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  device_tree.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/device_tree.c b/device_tree.c
>>> index 296278e12a..f8b46b3c73 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>>                       filename_path);
>>>          goto fail;
>>>      }
>>> +    if (dt_size > INT_MAX / 2 - 10000) {
>>
>> We should avoid magic number duplication.
>> That said, this patch looks safe.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks!
> 
>> BTW how did you figure that out?
> 
> Downstream handling of upstream commit da885fe1ee8 led me to the
> function.  I spotted dt_size = get_image_size(filename_path).
> Experience has taught me to check the left hand side's type.  Bad.  Then
> I saw how dt_size gets increased.  Worse.

So you genuinely neglected to mention Kurtis Miller then :)

>>> +        error_report("Device tree file '%s' is too large", filename_path);
>>> +        goto fail;
>>> +    }
>>>  
>>>      /* Expand to 2x size to give enough room for manipulation.  */
>>>      dt_size += 10000;
>>>

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Philippe Mathieu-Daudé]

On 4/10/19 7:28 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>> On 4/9/19 7:40 PM, Markus Armbruster wrote:
>>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>>> computation of @dt_size overflows to a negative number, which then
>>> gets converted to a very large size_t for g_malloc0() and
>>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>>> succeeds and load_image_size() survives, we'd assign the negative
>>> number to *sizep.  What that would do to the callers I can't say, but
>>> it's unlikely to be good.
>>>
>>> Fix by rejecting images whose size would overflow.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  device_tree.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/device_tree.c b/device_tree.c
>>> index 296278e12a..f8b46b3c73 100644
>>> --- a/device_tree.c
>>> +++ b/device_tree.c
>>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>>                       filename_path);
>>>          goto fail;
>>>      }
>>> +    if (dt_size > INT_MAX / 2 - 10000) {
>>
>> We should avoid magic number duplication.
>> That said, this patch looks safe.
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> Thanks!
> 
>> BTW how did you figure that out?
> 
> Downstream handling of upstream commit da885fe1ee8 led me to the
> function.  I spotted dt_size = get_image_size(filename_path).
> Experience has taught me to check the left hand side's type.  Bad.  Then
> I saw how dt_size gets increased.  Worse.

So you genuinely neglected to mention Kurtis Miller then :)

>>> +        error_report("Device tree file '%s' is too large", filename_path);
>>> +        goto fail;
>>> +    }
>>>  
>>>      /* Expand to 2x size to give enough room for manipulation.  */
>>>      dt_size += 10000;
>>>

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/10/19 7:28 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>> On 4/9/19 7:40 PM, Markus Armbruster wrote:
>>>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>>>> computation of @dt_size overflows to a negative number, which then
>>>> gets converted to a very large size_t for g_malloc0() and
>>>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>>>> succeeds and load_image_size() survives, we'd assign the negative
>>>> number to *sizep.  What that would do to the callers I can't say, but
>>>> it's unlikely to be good.
>>>>
>>>> Fix by rejecting images whose size would overflow.
>>>>
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>> ---
>>>>  device_tree.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/device_tree.c b/device_tree.c
>>>> index 296278e12a..f8b46b3c73 100644
>>>> --- a/device_tree.c
>>>> +++ b/device_tree.c
>>>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>>>                       filename_path);
>>>>          goto fail;
>>>>      }
>>>> +    if (dt_size > INT_MAX / 2 - 10000) {
>>>
>>> We should avoid magic number duplication.
>>> That said, this patch looks safe.
>>>
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> 
>> Thanks!
>> 
>>> BTW how did you figure that out?
>> 
>> Downstream handling of upstream commit da885fe1ee8 led me to the
>> function.  I spotted dt_size = get_image_size(filename_path).
>> Experience has taught me to check the left hand side's type.  Bad.  Then
>> I saw how dt_size gets increased.  Worse.
>
> So you genuinely neglected to mention Kurtis Miller then :)

Explanation, not excuse: the only occurence of the name in my downstream
reading was a two-liner BZ comment, which I totally missed in my haste
to give the fix a chance to make 4.0.  I certainly didn't mean to
deprive him of credit!

[...]

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Alistair Francis]

On Tue, Apr 9, 2019 at 10:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > On 4/10/19 7:28 AM, Markus Armbruster wrote:
> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> >>> On 4/9/19 7:40 PM, Markus Armbruster wrote:
> >>>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> >>>> computation of @dt_size overflows to a negative number, which then
> >>>> gets converted to a very large size_t for g_malloc0() and
> >>>> load_image_size().  In the (fortunately improbable) case g_malloc0()
> >>>> succeeds and load_image_size() survives, we'd assign the negative
> >>>> number to *sizep.  What that would do to the callers I can't say, but
> >>>> it's unlikely to be good.
> >>>>
> >>>> Fix by rejecting images whose size would overflow.
> >>>>
> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>>> ---
> >>>>  device_tree.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/device_tree.c b/device_tree.c
> >>>> index 296278e12a..f8b46b3c73 100644
> >>>> --- a/device_tree.c
> >>>> +++ b/device_tree.c
> >>>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
> >>>>                       filename_path);
> >>>>          goto fail;
> >>>>      }
> >>>> +    if (dt_size > INT_MAX / 2 - 10000) {
> >>>
> >>> We should avoid magic number duplication.
> >>> That said, this patch looks safe.
> >>>
> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>
> >> Thanks!
> >>
> >>> BTW how did you figure that out?
> >>
> >> Downstream handling of upstream commit da885fe1ee8 led me to the
> >> function.  I spotted dt_size = get_image_size(filename_path).
> >> Experience has taught me to check the left hand side's type.  Bad.  Then
> >> I saw how dt_size gets increased.  Worse.
> >
> > So you genuinely neglected to mention Kurtis Miller then :)
>
> Explanation, not excuse: the only occurence of the name in my downstream
> reading was a two-liner BZ comment, which I totally missed in my haste
> to give the fix a chance to make 4.0.  I certainly didn't mean to
> deprive him of credit!

No worries. I have sent the pull request and it includes the reported by.

Alistair

>
> [...]
>

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Alistair Francis]

On Tue, Apr 9, 2019 at 10:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > On 4/10/19 7:28 AM, Markus Armbruster wrote:
> >> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> >>> On 4/9/19 7:40 PM, Markus Armbruster wrote:
> >>>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
> >>>> computation of @dt_size overflows to a negative number, which then
> >>>> gets converted to a very large size_t for g_malloc0() and
> >>>> load_image_size().  In the (fortunately improbable) case g_malloc0()
> >>>> succeeds and load_image_size() survives, we'd assign the negative
> >>>> number to *sizep.  What that would do to the callers I can't say, but
> >>>> it's unlikely to be good.
> >>>>
> >>>> Fix by rejecting images whose size would overflow.
> >>>>
> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>>> ---
> >>>>  device_tree.c | 4 ++++
> >>>>  1 file changed, 4 insertions(+)
> >>>>
> >>>> diff --git a/device_tree.c b/device_tree.c
> >>>> index 296278e12a..f8b46b3c73 100644
> >>>> --- a/device_tree.c
> >>>> +++ b/device_tree.c
> >>>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
> >>>>                       filename_path);
> >>>>          goto fail;
> >>>>      }
> >>>> +    if (dt_size > INT_MAX / 2 - 10000) {
> >>>
> >>> We should avoid magic number duplication.
> >>> That said, this patch looks safe.
> >>>
> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >>
> >> Thanks!
> >>
> >>> BTW how did you figure that out?
> >>
> >> Downstream handling of upstream commit da885fe1ee8 led me to the
> >> function.  I spotted dt_size = get_image_size(filename_path).
> >> Experience has taught me to check the left hand side's type.  Bad.  Then
> >> I saw how dt_size gets increased.  Worse.
> >
> > So you genuinely neglected to mention Kurtis Miller then :)
>
> Explanation, not excuse: the only occurence of the name in my downstream
> reading was a two-liner BZ comment, which I totally missed in my haste
> to give the fix a chance to make 4.0.  I certainly didn't mean to
> deprive him of credit!

No worries. I have sent the pull request and it includes the reported by.

Alistair

>
> [...]
>

[Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Philippe Mathieu-Daudé]

On 4/10/19 8:34 AM, Alistair Francis wrote:
> On Tue, Apr 9, 2019 at 10:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>> On 4/10/19 7:28 AM, Markus Armbruster wrote:
>>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>>> On 4/9/19 7:40 PM, Markus Armbruster wrote:
>>>>>> If the value of get_image_size() exceeds INT_MAX / 2 - 10000, the
>>>>>> computation of @dt_size overflows to a negative number, which then
>>>>>> gets converted to a very large size_t for g_malloc0() and
>>>>>> load_image_size().  In the (fortunately improbable) case g_malloc0()
>>>>>> succeeds and load_image_size() survives, we'd assign the negative
>>>>>> number to *sizep.  What that would do to the callers I can't say, but
>>>>>> it's unlikely to be good.
>>>>>>
>>>>>> Fix by rejecting images whose size would overflow.
>>>>>>
>>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>>> ---
>>>>>>  device_tree.c | 4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/device_tree.c b/device_tree.c
>>>>>> index 296278e12a..f8b46b3c73 100644
>>>>>> --- a/device_tree.c
>>>>>> +++ b/device_tree.c
>>>>>> @@ -84,6 +84,10 @@ void *load_device_tree(const char *filename_path, int *sizep)
>>>>>>                       filename_path);
>>>>>>          goto fail;
>>>>>>      }
>>>>>> +    if (dt_size > INT_MAX / 2 - 10000) {
>>>>>
>>>>> We should avoid magic number duplication.
>>>>> That said, this patch looks safe.
>>>>>
>>>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>>
>>>> Thanks!
>>>>
>>>>> BTW how did you figure that out?
>>>>
>>>> Downstream handling of upstream commit da885fe1ee8 led me to the
>>>> function.  I spotted dt_size = get_image_size(filename_path).
>>>> Experience has taught me to check the left hand side's type.  Bad.  Then
>>>> I saw how dt_size gets increased.  Worse.
>>>
>>> So you genuinely neglected to mention Kurtis Miller then :)
>>
>> Explanation, not excuse: the only occurence of the name in my downstream
>> reading was a two-liner BZ comment, which I totally missed in my haste
>> to give the fix a chance to make 4.0.  I certainly didn't mean to
>> deprive him of credit!

My English teacher explained me neglected sounds like a
reprimand/reproach (as in negligence), sorry I didn't mean to be rude
here, I wanted to say something like "Peter remarked you did gave
credits to the first one who reported this issue, but since you figured
this bug via your own way, the omission was not intentional then."

> No worries. I have sent the pull request and it includes the reported by.
> 
> Alistair
> 
>>
>> [...]
>>

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/10/19 8:34 AM, Alistair Francis wrote:
>> On Tue, Apr 9, 2019 at 10:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>> On 4/10/19 7:28 AM, Markus Armbruster wrote:
>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
[...]
>>>>>> BTW how did you figure that out?
>>>>>
>>>>> Downstream handling of upstream commit da885fe1ee8 led me to the
>>>>> function.  I spotted dt_size = get_image_size(filename_path).
>>>>> Experience has taught me to check the left hand side's type.  Bad.  Then
>>>>> I saw how dt_size gets increased.  Worse.
>>>>
>>>> So you genuinely neglected to mention Kurtis Miller then :)
>>>
>>> Explanation, not excuse: the only occurence of the name in my downstream
>>> reading was a two-liner BZ comment, which I totally missed in my haste
>>> to give the fix a chance to make 4.0.  I certainly didn't mean to
>>> deprive him of credit!
>
> My English teacher explained me neglected sounds like a
> reprimand/reproach (as in negligence), sorry I didn't mean to be rude
> here, I wanted to say something like "Peter remarked you did gave
> credits to the first one who reported this issue, but since you figured
> this bug via your own way, the omission was not intentional then."

Don't worry, my social interactions teacher taught me to assume good
intent ;)

[...]

Re: [Qemu-devel] [PATCH for-4.0-maybe] device_tree: Fix integer overflowing in load_device_tree()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 4/10/19 8:34 AM, Alistair Francis wrote:
>> On Tue, Apr 9, 2019 at 10:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>> On 4/10/19 7:28 AM, Markus Armbruster wrote:
>>>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
[...]
>>>>>> BTW how did you figure that out?
>>>>>
>>>>> Downstream handling of upstream commit da885fe1ee8 led me to the
>>>>> function.  I spotted dt_size = get_image_size(filename_path).
>>>>> Experience has taught me to check the left hand side's type.  Bad.  Then
>>>>> I saw how dt_size gets increased.  Worse.
>>>>
>>>> So you genuinely neglected to mention Kurtis Miller then :)
>>>
>>> Explanation, not excuse: the only occurence of the name in my downstream
>>> reading was a two-liner BZ comment, which I totally missed in my haste
>>> to give the fix a chance to make 4.0.  I certainly didn't mean to
>>> deprive him of credit!
>
> My English teacher explained me neglected sounds like a
> reprimand/reproach (as in negligence), sorry I didn't mean to be rude
> here, I wanted to say something like "Peter remarked you did gave
> credits to the first one who reported this issue, but since you figured
> this bug via your own way, the omission was not intentional then."

Don't worry, my social interactions teacher taught me to assume good
intent ;)

[...]