[U-Boot] [PATCH] Revert "part: Allocate only one legacy_mbr buffer"
faiz_abbas at ti.com
Wed Sep 4 12:49:25 UTC 2019
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,
> 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.
More information about the U-Boot