[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 <address@hidden> wrote:
>> Philippe Mathieu-Daudé <address@hidden> writes:
>>> On 4/10/19 7:28 AM, Markus Armbruster wrote:
>>>> Philippe Mathieu-Daudé <address@hidden> 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 <address@hidden>
>>>>>> ---
>>>>>>  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é <address@hidden>
>>>>
>>>> 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
> 
>>
>> [...]
>>