From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Andreas_F=c3=a4rber?= Date: Wed, 13 Apr 2016 13:26:35 +0200 Subject: [U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows In-Reply-To: <593768F6-C54A-4B70-BC48-FBAB1778ADC0@suse.de> References: <1460517876-4435-1-git-send-email-afaerber@suse.de> <593768F6-C54A-4B70-BC48-FBAB1778ADC0@suse.de> Message-ID: <570E2CEB.7060008@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 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? 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. >> 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). 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... >> efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false); >> >> /* Add Runtime Services */ >> -- >> 2.6.6 Thanks, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg)