From owner-svn-src-releng@freebsd.org Tue Nov 12 18:10:27 2019 Return-Path: Delivered-To: svn-src-releng@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id BC6921B43A3; Tue, 12 Nov 2019 18:10:27 +0000 (UTC) (envelope-from gordon@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 47CG4g5JV9z3McB; Tue, 12 Nov 2019 18:10:27 +0000 (UTC) (envelope-from gordon@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 992343A1C; Tue, 12 Nov 2019 18:10:27 +0000 (UTC) (envelope-from gordon@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id xACIAR9J086978; Tue, 12 Nov 2019 18:10:27 GMT (envelope-from gordon@FreeBSD.org) Received: (from gordon@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id xACIAQtI086973; Tue, 12 Nov 2019 18:10:26 GMT (envelope-from gordon@FreeBSD.org) Message-Id: <201911121810.xACIAQtI086973@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: gordon set sender to gordon@FreeBSD.org using -f From: Gordon Tetlow Date: Tue, 12 Nov 2019 18:10:26 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-releng@freebsd.org Subject: svn commit: r354652 - in releng: 12.0/stand/efi/loader 12.1/stand/efi/loader X-SVN-Group: releng X-SVN-Commit-Author: gordon X-SVN-Commit-Paths: in releng: 12.0/stand/efi/loader 12.1/stand/efi/loader X-SVN-Commit-Revision: 354652 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-releng@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the release engineering / security commits to the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Nov 2019 18:10:27 -0000 Author: gordon Date: Tue Nov 12 18:10:26 2019 New Revision: 354652 URL: https://svnweb.freebsd.org/changeset/base/354652 Log: Fix UEFI Loader Memory Fragmentation. Approved by: so Security: FreeBSD-EN-19:19.loader Modified: releng/12.0/stand/efi/loader/bootinfo.c releng/12.0/stand/efi/loader/copy.c releng/12.1/stand/efi/loader/bootinfo.c releng/12.1/stand/efi/loader/copy.c Modified: releng/12.0/stand/efi/loader/bootinfo.c ============================================================================== --- releng/12.0/stand/efi/loader/bootinfo.c Tue Nov 12 18:04:28 2019 (r354651) +++ releng/12.0/stand/efi/loader/bootinfo.c Tue Nov 12 18:10:26 2019 (r354652) @@ -287,12 +287,12 @@ static int bi_load_efi_data(struct preloaded_file *kfp) { EFI_MEMORY_DESCRIPTOR *mm; - EFI_PHYSICAL_ADDRESS addr; + EFI_PHYSICAL_ADDRESS addr = 0; EFI_STATUS status; const char *efi_novmap; size_t efisz; UINTN efi_mapkey; - UINTN mmsz, pages, retry, sz; + UINTN dsz, pages, retry, sz; UINT32 mmver; struct efi_map_header *efihdr; bool do_vmap; @@ -323,76 +323,94 @@ bi_load_efi_data(struct preloaded_file *kfp) efisz = (sizeof(struct efi_map_header) + 0xf) & ~0xf; /* - * Assgin size of EFI_MEMORY_DESCRIPTOR to keep compatible with + * Assign size of EFI_MEMORY_DESCRIPTOR to keep compatible with * u-boot which doesn't fill this value when buffer for memory * descriptors is too small (eg. 0 to obtain memory map size) */ - mmsz = sizeof(EFI_MEMORY_DESCRIPTOR); + dsz = sizeof(EFI_MEMORY_DESCRIPTOR); /* - * It is possible that the first call to ExitBootServices may change - * the map key. Fetch a new map key and retry ExitBootServices in that - * case. + * Allocate enough pages to hold the bootinfo block and the + * memory map EFI will return to us. The memory map has an + * unknown size, so we have to determine that first. Note that + * the AllocatePages call can itself modify the memory map, so + * we have to take that into account as well. The changes to + * the memory map are caused by splitting a range of free + * memory into two, so that one is marked as being loader + * data. */ + + sz = 0; + + /* + * Matthew Garrett has observed at least one system changing the + * memory map when calling ExitBootServices, causing it to return an + * error, probably because callbacks are allocating memory. + * So we need to retry calling it at least once. + */ for (retry = 2; retry > 0; retry--) { - /* - * Allocate enough pages to hold the bootinfo block and the - * memory map EFI will return to us. The memory map has an - * unknown size, so we have to determine that first. Note that - * the AllocatePages call can itself modify the memory map, so - * we have to take that into account as well. The changes to - * the memory map are caused by splitting a range of free - * memory into two (AFAICT), so that one is marked as being - * loader data. - */ - sz = 0; - BS->GetMemoryMap(&sz, NULL, &efi_mapkey, &mmsz, &mmver); - sz += mmsz; - sz = (sz + 0xf) & ~0xf; - pages = EFI_SIZE_TO_PAGES(sz + efisz); - status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, - pages, &addr); - if (EFI_ERROR(status)) { - printf("%s: AllocatePages error %lu\n", __func__, - EFI_ERROR_CODE(status)); - return (ENOMEM); - } + for (;;) { + status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &dsz, &mmver); + if (!EFI_ERROR(status)) + break; - /* - * Read the memory map and stash it after bootinfo. Align the - * memory map on a 16-byte boundary (the bootinfo block is page - * aligned). - */ - efihdr = (struct efi_map_header *)(uintptr_t)addr; - mm = (void *)((uint8_t *)efihdr + efisz); - sz = (EFI_PAGE_SIZE * pages) - efisz; + if (status != EFI_BUFFER_TOO_SMALL) { + printf("%s: GetMemoryMap error %lu\n", __func__, + EFI_ERROR_CODE(status)); + return (EINVAL); + } - status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &mmsz, &mmver); - if (EFI_ERROR(status)) { - printf("%s: GetMemoryMap error %lu\n", __func__, - EFI_ERROR_CODE(status)); - return (EINVAL); - } - status = BS->ExitBootServices(IH, efi_mapkey); - if (EFI_ERROR(status) == 0) { + if (addr != 0) + BS->FreePages(addr, pages); + + /* Add 10 descriptors to the size to allow for + * fragmentation caused by calling AllocatePages */ + sz += (10 * dsz); + pages = EFI_SIZE_TO_PAGES(sz + efisz); + status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, + pages, &addr); + if (EFI_ERROR(status)) { + printf("%s: AllocatePages error %lu\n", __func__, + EFI_ERROR_CODE(status)); + return (ENOMEM); + } + /* - * This may be disabled by setting efi_disable_vmap in - * loader.conf(5). By default we will setup the virtual - * map entries. + * Read the memory map and stash it after bootinfo. Align the + * memory map on a 16-byte boundary (the bootinfo block is page + * aligned). */ - if (do_vmap) - efi_do_vmap(mm, sz, mmsz, mmver); - efihdr->memory_size = sz; - efihdr->descriptor_size = mmsz; - efihdr->descriptor_version = mmver; - file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, - efihdr); - return (0); + efihdr = (struct efi_map_header *)(uintptr_t)addr; + mm = (void *)((uint8_t *)efihdr + efisz); + sz = (EFI_PAGE_SIZE * pages) - efisz; } + + status = BS->ExitBootServices(IH, efi_mapkey); + if (!EFI_ERROR(status)) + break; + } + + if (retry == 0) { BS->FreePages(addr, pages); + printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status)); + return (EINVAL); } - printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status)); - return (EINVAL); + + /* + * This may be disabled by setting efi_disable_vmap in + * loader.conf(5). By default we will setup the virtual + * map entries. + */ + + if (do_vmap) + efi_do_vmap(mm, sz, dsz, mmver); + efihdr->memory_size = sz; + efihdr->descriptor_size = dsz; + efihdr->descriptor_version = mmver; + file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, + efihdr); + + return (0); } /* Modified: releng/12.0/stand/efi/loader/copy.c ============================================================================== --- releng/12.0/stand/efi/loader/copy.c Tue Nov 12 18:04:28 2019 (r354651) +++ releng/12.0/stand/efi/loader/copy.c Tue Nov 12 18:10:26 2019 (r354652) @@ -95,7 +95,7 @@ static void efi_verify_staging_size(unsigned long *nr_pages) { UINTN sz; - EFI_MEMORY_DESCRIPTOR *map, *p; + EFI_MEMORY_DESCRIPTOR *map = NULL, *p; EFI_PHYSICAL_ADDRESS start, end; UINTN key, dsz; UINT32 dver; @@ -104,17 +104,28 @@ efi_verify_staging_size(unsigned long *nr_pages) unsigned long available_pages = 0; sz = 0; - status = BS->GetMemoryMap(&sz, 0, &key, &dsz, &dver); - if (status != EFI_BUFFER_TOO_SMALL) { - printf("Can't determine memory map size\n"); - return; - } - map = malloc(sz); - status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver); - if (EFI_ERROR(status)) { - printf("Can't read memory map\n"); - goto out; + for (;;) { + status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver); + if (!EFI_ERROR(status)) + break; + + if (status != EFI_BUFFER_TOO_SMALL) { + printf("Can't read memory map: %lu\n", + EFI_ERROR_CODE(status)); + goto out; + } + + free(map); + + /* Allocate 10 descriptors more than the size reported, + * to allow for any fragmentation caused by calling + * malloc */ + map = malloc(sz + (10 * dsz)); + if (map == NULL) { + printf("Unable to allocate memory\n"); + goto out; + } } ndesc = sz / dsz; Modified: releng/12.1/stand/efi/loader/bootinfo.c ============================================================================== --- releng/12.1/stand/efi/loader/bootinfo.c Tue Nov 12 18:04:28 2019 (r354651) +++ releng/12.1/stand/efi/loader/bootinfo.c Tue Nov 12 18:10:26 2019 (r354652) @@ -287,12 +287,12 @@ static int bi_load_efi_data(struct preloaded_file *kfp) { EFI_MEMORY_DESCRIPTOR *mm; - EFI_PHYSICAL_ADDRESS addr; + EFI_PHYSICAL_ADDRESS addr = 0; EFI_STATUS status; const char *efi_novmap; size_t efisz; UINTN efi_mapkey; - UINTN mmsz, pages, retry, sz; + UINTN dsz, pages, retry, sz; UINT32 mmver; struct efi_map_header *efihdr; bool do_vmap; @@ -323,76 +323,94 @@ bi_load_efi_data(struct preloaded_file *kfp) efisz = (sizeof(struct efi_map_header) + 0xf) & ~0xf; /* - * Assgin size of EFI_MEMORY_DESCRIPTOR to keep compatible with + * Assign size of EFI_MEMORY_DESCRIPTOR to keep compatible with * u-boot which doesn't fill this value when buffer for memory * descriptors is too small (eg. 0 to obtain memory map size) */ - mmsz = sizeof(EFI_MEMORY_DESCRIPTOR); + dsz = sizeof(EFI_MEMORY_DESCRIPTOR); /* - * It is possible that the first call to ExitBootServices may change - * the map key. Fetch a new map key and retry ExitBootServices in that - * case. + * Allocate enough pages to hold the bootinfo block and the + * memory map EFI will return to us. The memory map has an + * unknown size, so we have to determine that first. Note that + * the AllocatePages call can itself modify the memory map, so + * we have to take that into account as well. The changes to + * the memory map are caused by splitting a range of free + * memory into two, so that one is marked as being loader + * data. */ + + sz = 0; + + /* + * Matthew Garrett has observed at least one system changing the + * memory map when calling ExitBootServices, causing it to return an + * error, probably because callbacks are allocating memory. + * So we need to retry calling it at least once. + */ for (retry = 2; retry > 0; retry--) { - /* - * Allocate enough pages to hold the bootinfo block and the - * memory map EFI will return to us. The memory map has an - * unknown size, so we have to determine that first. Note that - * the AllocatePages call can itself modify the memory map, so - * we have to take that into account as well. The changes to - * the memory map are caused by splitting a range of free - * memory into two (AFAICT), so that one is marked as being - * loader data. - */ - sz = 0; - BS->GetMemoryMap(&sz, NULL, &efi_mapkey, &mmsz, &mmver); - sz += mmsz; - sz = (sz + 0xf) & ~0xf; - pages = EFI_SIZE_TO_PAGES(sz + efisz); - status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, - pages, &addr); - if (EFI_ERROR(status)) { - printf("%s: AllocatePages error %lu\n", __func__, - EFI_ERROR_CODE(status)); - return (ENOMEM); - } + for (;;) { + status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &dsz, &mmver); + if (!EFI_ERROR(status)) + break; - /* - * Read the memory map and stash it after bootinfo. Align the - * memory map on a 16-byte boundary (the bootinfo block is page - * aligned). - */ - efihdr = (struct efi_map_header *)(uintptr_t)addr; - mm = (void *)((uint8_t *)efihdr + efisz); - sz = (EFI_PAGE_SIZE * pages) - efisz; + if (status != EFI_BUFFER_TOO_SMALL) { + printf("%s: GetMemoryMap error %lu\n", __func__, + EFI_ERROR_CODE(status)); + return (EINVAL); + } - status = BS->GetMemoryMap(&sz, mm, &efi_mapkey, &mmsz, &mmver); - if (EFI_ERROR(status)) { - printf("%s: GetMemoryMap error %lu\n", __func__, - EFI_ERROR_CODE(status)); - return (EINVAL); - } - status = BS->ExitBootServices(IH, efi_mapkey); - if (EFI_ERROR(status) == 0) { + if (addr != 0) + BS->FreePages(addr, pages); + + /* Add 10 descriptors to the size to allow for + * fragmentation caused by calling AllocatePages */ + sz += (10 * dsz); + pages = EFI_SIZE_TO_PAGES(sz + efisz); + status = BS->AllocatePages(AllocateAnyPages, EfiLoaderData, + pages, &addr); + if (EFI_ERROR(status)) { + printf("%s: AllocatePages error %lu\n", __func__, + EFI_ERROR_CODE(status)); + return (ENOMEM); + } + /* - * This may be disabled by setting efi_disable_vmap in - * loader.conf(5). By default we will setup the virtual - * map entries. + * Read the memory map and stash it after bootinfo. Align the + * memory map on a 16-byte boundary (the bootinfo block is page + * aligned). */ - if (do_vmap) - efi_do_vmap(mm, sz, mmsz, mmver); - efihdr->memory_size = sz; - efihdr->descriptor_size = mmsz; - efihdr->descriptor_version = mmver; - file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, - efihdr); - return (0); + efihdr = (struct efi_map_header *)(uintptr_t)addr; + mm = (void *)((uint8_t *)efihdr + efisz); + sz = (EFI_PAGE_SIZE * pages) - efisz; } + + status = BS->ExitBootServices(IH, efi_mapkey); + if (!EFI_ERROR(status)) + break; + } + + if (retry == 0) { BS->FreePages(addr, pages); + printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status)); + return (EINVAL); } - printf("ExitBootServices error %lu\n", EFI_ERROR_CODE(status)); - return (EINVAL); + + /* + * This may be disabled by setting efi_disable_vmap in + * loader.conf(5). By default we will setup the virtual + * map entries. + */ + + if (do_vmap) + efi_do_vmap(mm, sz, dsz, mmver); + efihdr->memory_size = sz; + efihdr->descriptor_size = dsz; + efihdr->descriptor_version = mmver; + file_addmetadata(kfp, MODINFOMD_EFI_MAP, efisz + sz, + efihdr); + + return (0); } /* Modified: releng/12.1/stand/efi/loader/copy.c ============================================================================== --- releng/12.1/stand/efi/loader/copy.c Tue Nov 12 18:04:28 2019 (r354651) +++ releng/12.1/stand/efi/loader/copy.c Tue Nov 12 18:10:26 2019 (r354652) @@ -95,7 +95,7 @@ static void efi_verify_staging_size(unsigned long *nr_pages) { UINTN sz; - EFI_MEMORY_DESCRIPTOR *map, *p; + EFI_MEMORY_DESCRIPTOR *map = NULL, *p; EFI_PHYSICAL_ADDRESS start, end; UINTN key, dsz; UINT32 dver; @@ -104,17 +104,28 @@ efi_verify_staging_size(unsigned long *nr_pages) unsigned long available_pages = 0; sz = 0; - status = BS->GetMemoryMap(&sz, 0, &key, &dsz, &dver); - if (status != EFI_BUFFER_TOO_SMALL) { - printf("Can't determine memory map size\n"); - return; - } - map = malloc(sz); - status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver); - if (EFI_ERROR(status)) { - printf("Can't read memory map\n"); - goto out; + for (;;) { + status = BS->GetMemoryMap(&sz, map, &key, &dsz, &dver); + if (!EFI_ERROR(status)) + break; + + if (status != EFI_BUFFER_TOO_SMALL) { + printf("Can't read memory map: %lu\n", + EFI_ERROR_CODE(status)); + goto out; + } + + free(map); + + /* Allocate 10 descriptors more than the size reported, + * to allow for any fragmentation caused by calling + * malloc */ + map = malloc(sz + (10 * dsz)); + if (map == NULL) { + printf("Unable to allocate memory\n"); + goto out; + } } ndesc = sz / dsz;