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