Hi Alexey, On 04/09/19 5:16 PM, Alexey Brodkin wrote: > Hi Faiz, > >> -----Original Message----- >> From: Faiz Abbas <faiz_ab...@ti.com> >> Sent: Wednesday, September 4, 2019 2:44 PM >> To: Alexey Brodkin <abrod...@synopsys.com> >> Cc: paule...@forallsecure.com; tr...@konsulko.com; u-boot@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_ab...@ti.com> >>>> Sent: Wednesday, September 4, 2019 12:22 PM >>>> To: u-boot@lists.denx.de >>>> Cc: paule...@forallsecure.com; faiz_ab...@ti.com; Alexey Brodkin >>>> <abrod...@synopsys.com>; >>>> tr...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
