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
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;
>
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;
>>
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;
>>>
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!
[...]
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
>
> [...]
>
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
>
>>
>> [...]
>>
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 ;) [...]
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
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
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 >
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
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
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.
© 2016 - 2024 Red Hat, Inc.