[U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer"

[Faiz Abbas]

Hi Alexey,

On 04/09/19 5:16 PM, Alexey Brodkin wrote:
> Hi Faiz,
> 
>> -----Original Message-----
>> From: Faiz Abbas <faiz_abbas at ti.com>
>> Sent: Wednesday, September 4, 2019 2:44 PM
>> To: Alexey Brodkin <abrodkin at synopsys.com>
>> Cc: paulemge at forallsecure.com; trini at konsulko.com; u-boot at lists.denx.de
>> Subject: Re: [PATCH] Revert "part: Allocate only one legacy_mbr buffer"
>>
>> Hi Alexey,
>>
>> On 04/09/19 3:42 PM, Alexey Brodkin wrote:
>>> Hi Faiz,
>>>
>>>> -----Original Message-----
>>>> From: Faiz Abbas <faiz_abbas at ti.com>
>>>> Sent: Wednesday, September 4, 2019 12:22 PM
>>>> To: u-boot at lists.denx.de
>>>> Cc: paulemge at forallsecure.com; faiz_abbas at ti.com; Alexey Brodkin <abrodkin at synopsys.com>;
>>>> trini at konsulko.com
>>>> Subject: [PATCH] Revert "part: Allocate only one legacy_mbr buffer"
>>>>
>>>> This reverts commit 8639e34d2c5e12cc2e45c95b1a2e97c22bf6a711.
>>>>
>>>> The blk_dread() call following the allocation will read one block from
>>>> the device. This will lead to overflow if the blocksize is greater than
>>>> the size of legacy_mbr. Fix this by allocating one block size.
>>>
>>> Did you read justification of my change that you're reverting now?
>>> With your change in place we'll allocate a buffer
>>> of size = (sizeof(legacy_mbr) * dev_desc->blksz).
>>>
>>> Is that something you really want?
>>
>> Oops. You're right. I thought your patch was changing buffer size from
>> blksz to legacy_mbr.
>>
>>>
>>> I guess what you really want to do is to allocate buffer for "mbr"
>>> dynamically of size which is max(sizeof(legacy_mbr), dev_desc->blksz).
>>>
>>
>> With the assumption that blksz is always greater than
>> sizeof(legacy_mbr), this should work:
>>
>> ALLOC_CACHE_ALIGN_BUFFER(legacy_mbr, mbr, DIV_ROUND_UP(dev_desc->blksz,
>> sizeof(legacy_mbr)));
> 
> No this won't work. See ALLOC_CACHE_ALIGN_BUFFER does static allocation
> in compile-time while blksz is set in runtime.
> 
> That's why I mentioned switch to runtime allocation.
> 

Hmm. So the problem isn't as much about allocating too much in the
buffer but about using dynamically determined size for static array
declaration. This is being used all over this disk/part_dos.c file.
Shouldn't we fix all of that? Not sure how it has been working all along.

Thanks,
Faiz