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

[Alexey Brodkin]

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.

-Alexey