From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexander Graf Date: Wed, 13 Apr 2016 13:48:49 +0200 Subject: [U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows In-Reply-To: <570E2CEB.7060008@suse.de> References: <1460517876-4435-1-git-send-email-afaerber@suse.de> <593768F6-C54A-4B70-BC48-FBAB1778ADC0@suse.de> <570E2CEB.7060008@suse.de> Message-ID: <570E3221.6090607@suse.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 04/13/2016 01:26 PM, Andreas F?rber wrote: > Am 13.04.2016 um 07:50 schrieb Alexander Graf: >>> Am 13.04.2016 um 05:24 schrieb Andreas F?rber : >>> >>> jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be zero. >>> Handle this by replacing it with 0x100000000 in that case. >> Nice catch! >> >>> Cc: Alexander Graf >>> Signed-off-by: Andreas F?rber >>> --- >>> lib/efi_loader/efi_memory.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>> index 138736f..7b87108 100644 >>> --- a/lib/efi_loader/efi_memory.c >>> +++ b/lib/efi_loader/efi_memory.c >>> @@ -225,7 +225,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type, >>> switch (type) { >>> case 0: >>> /* Any page */ >>> - addr = efi_find_free_memory(len, gd->ram_top); >>> + addr = efi_find_free_memory(len, gd->ram_top == 0 ? 0x100000000ull : gd->ram_top); > If we do use a constant, we should probably be using > UINT64_C(0x100000000) rather than ull for compatibility. > >> Couldn't we just use gd->ram_top - 1? Then we underflow to 0xffffffff and everything should just work. > Hm, that does work in my testing. Is it guaranteed it will handle that > as unsigned long rather than uint64_t? And did you mean to always use it > that way or just in the zero case? I.e., might we be wasting one byte in > the non-zero case or is it guaranteed that the top of RAM is always > reserved? Top of RAM is where U-Boot lives, so it's always reserved anyways :). > I was wondering whether we might run into the same overflow problem on > aarch64, in which case my hunk would be wrong, but your -1 should work. I'm not aware of an aarch64 implementation (or even spec) that would allow for 64bit physical address space, so we're safe on aarch64. But I like the -1 better, since it just completely negates any overflow. > >>> if (!addr) { >>> r = EFI_NOT_FOUND; >>> break; >>> @@ -343,7 +343,7 @@ int efi_memory_init(void) >>> >>> /* Add U-Boot */ >>> uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; >>> - uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >>> + uboot_pages = ((gd->ram_top == 0 ? 0x100000000ull : gd->ram_top) - uboot_start) >> EFI_PAGE_SHIFT; >> Are you sure this hunk is necessary? We should already underflow to the correct value here. > Positive that _something_ here is necessary for sanity, it was using a > huge number of pages (uboot_start is uint64_t). Ok, so the fix would be to change uboot_start to unsigned long to have the same type as gd->ram_top. > > If we use an odd number like -1, then we probably should add > EFI_PAGE_MASK as done for the RAM for the non-zero case, shouldn't we? > That might overflow on aarch64 though... Since we shift afterwards we don't need to mask anything. Bits that we would mask away are already gone after the shift ;). Does the patch below work? Alex diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index df99585..71a3d19 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -220,7 +220,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type, switch (type) { case 0: /* Any page */ - addr = efi_find_free_memory(len, gd->ram_top); + addr = efi_find_free_memory(len, gd->start_addr_sp); if (!addr) { r = EFI_NOT_FOUND; break; @@ -320,9 +320,9 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size, int efi_memory_init(void) { - uint64_t runtime_start, runtime_end, runtime_pages; - uint64_t uboot_start, uboot_pages; - uint64_t uboot_stack_size = 16 * 1024 * 1024; + unsigned long runtime_start, runtime_end, runtime_pages; + unsigned long uboot_start, uboot_pages; + unsigned long uboot_stack_size = 16 * 1024 * 1024; int i; /* Add RAM */