From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) by mx.groups.io with SMTP id smtpd.web10.1398.1688055636938762346 for ; Thu, 29 Jun 2023 09:20:37 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="signature has expired" header.i=@taylorbeebe.com header.s=google header.b=SlWD7vDs; spf=pass (domain: taylorbeebe.com, ip: 209.85.210.177, mailfrom: t@taylorbeebe.com) Received: by mail-pf1-f177.google.com with SMTP id d2e1a72fcca58-666eb03457cso600901b3a.1 for ; Thu, 29 Jun 2023 09:20:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=taylorbeebe.com; s=google; t=1688055636; x=1690647636; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=hb0wxvwdFhl09p+3c+PIbovlLeKXbQb2X4MEzBMepyw=; b=SlWD7vDsE9nBSSXLbdLo98AdXPwOY+9XdkuJv2KSatH9ENMCDlwrpQZhX78q+FirPT UwiXYLeRjsD8stTkMR+Hz2NlYIAS9IurCcOrH2KNFWOYCRWYL7Vry9I3jAIIaNfBpF7t j4Sp0ak7gxb3MJTcjteI8RUrPGYxQSA8npWuyx/VAVNFhuATMPB7EovdQz1se648NA9s +3Z3tnxmqduuvUzNKIP0llQqCvn/N0j8jkQM/r/8arMkbnqucDKIGm9yJsp5YnbAj8sa R4lrNKLu6fFccIb6TK9wMAqj0BmyipTrLtXu1v1DAmfGe/ukVK6Q8sWO9xdmPIt3uY6p bYVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688055636; x=1690647636; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hb0wxvwdFhl09p+3c+PIbovlLeKXbQb2X4MEzBMepyw=; b=HHHOufq0ruvjXJ3niTK43hqqqOrvoUn16O0kMr7NsQ6TKmEq1BmF6lCVQtRpYjBRi6 DJ1B1a9/pSNCZpTNgkSE/1qaESgYaSxeYyJHVPfAGSWBmekROsL/bIMbh+9qW1LmMNAI 1TWwYNrQGjEiDUYs1EZ0K3T5kodCvuUSGpQsQrcvO3LvnVxhrPGESIxU5SvBGUmLvWJ5 IE9UhnPxemukhZshwWWaaxKBR0+6dzBONKVhiicsV32287m4P5vZcQnTKRBZym4/fw7Z 1QYvMb7Mat4Nco8FO8I6z7PddW7bHqDkaPivtfHfT/5FaF+fYIpWY1PuZq++UkNDs4Dv wMPg== X-Gm-Message-State: ABy/qLYP641Up7058ZRvZqHzON9gj2G3rKW9mZnCIkjSxw4D0Pcw5nSx etc/iZPHfh0SgudojJc8YYfzvFWpVHj09FuufBuXFA== X-Google-Smtp-Source: APBJJlFlEz623kFAdVpx0kYXFAbfm2ZWf0U6Np0ty71MM+mwySHW30TTbyvbvMVNymVRMm4y9++W3Q== X-Received: by 2002:a05:6a00:2346:b0:673:5d1e:6657 with SMTP id j6-20020a056a00234600b006735d1e6657mr471408pfj.7.1688055636262; Thu, 29 Jun 2023 09:20:36 -0700 (PDT) Return-Path: Received: from localhost.localdomain ([50.46.230.135]) by smtp.gmail.com with ESMTPSA id b5-20020aa78705000000b0064f7c56d8b7sm6993578pfo.219.2023.06.29.09.20.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Jun 2023 09:20:35 -0700 (PDT) From: "Taylor Beebe" To: devel@edk2.groups.io Cc: Taylor Beebe , Leif Lindholm , Ard Biesheuvel , Taylor Beebe Subject: [PATCH 3/4] ArmPkg: Fix Unsafe ASSERTs in MMU Logic Date: Thu, 29 Jun 2023 09:17:57 -0700 Message-ID: <580bf85d35a2f301cb5b99e88f58ac4af696cae1.1687989723.git.t@taylorbeebe.com> X-Mailer: git-send-email 2.41.0.windows.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Taylor Beebe =0D There are ASSERTs present in the MMU logic to ensure various=0D functions return successfully, but these ASSERTs may be ignored=0D on release builds causing unsafe behavior. This patch updates=0D the logic to handle unexpected return values and branch safely.=0D =0D Cc: Leif Lindholm =0D Cc: Ard Biesheuvel =0D =0D Signed-off-by: Taylor Beebe =0D ---=0D ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 21 ++++++++++++-----=0D ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 36 ++++++++++++++++++++++++-----=0D 2 files changed, 45 insertions(+), 12 deletions(-)=0D =0D diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AA= rch64/Mmu.c=0D index 0d3bc2809682..d9d386dbed6b 100644=0D --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c=0D +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c=0D @@ -148,10 +148,12 @@ GetNextEntryAttribute (=0D // Get the memory space map from GCD=0D MemorySpaceMap =3D NULL;=0D Status =3D gDS->GetMemorySpaceMap (&NumberOfDescriptors, &Memory= SpaceMap);=0D - ASSERT_EFI_ERROR (Status);=0D =0D - // We cannot get more than 3-level page table=0D - ASSERT (TableLevel <=3D 3);=0D + if (EFI_ERROR (Status) || (TableLevel > 3)) {=0D + ASSERT_EFI_ERROR (Status);=0D + ASSERT (TableLevel <=3D 3);=0D + return 0;=0D + }=0D =0D // While the top level table might not contain TT_ENTRY_COUNT entries;=0D // the subsequent ones should be filled up=0D @@ -243,7 +245,11 @@ SyncCacheConfig (=0D //=0D MemorySpaceMap =3D NULL;=0D Status =3D gDS->GetMemorySpaceMap (&NumberOfDescriptors, &Memory= SpaceMap);=0D - ASSERT_EFI_ERROR (Status);=0D +=0D + if (EFI_ERROR (Status)) {=0D + ASSERT_EFI_ERROR (Status);=0D + return Status;=0D + }=0D =0D // The GCD implementation maintains its own copy of the state of memory = space attributes. GCD needs=0D // to know what the initial memory space attributes are. The CPU Arch. = Protocol does not provide a=0D @@ -277,7 +283,7 @@ SyncCacheConfig (=0D );=0D =0D // Update GCD with the last region if valid=0D - if (PageAttribute !=3D INVALID_ENTRY) {=0D + if ((PageAttribute !=3D INVALID_ENTRY) && (EndAddressGcdRegion > BaseAdd= ressGcdRegion)) {=0D SetGcdMemorySpaceAttributes (=0D MemorySpaceMap,=0D NumberOfDescriptors,=0D @@ -430,7 +436,10 @@ GetMemoryRegion (=0D UINTN EntryCount;=0D UINTN T0SZ;=0D =0D - ASSERT ((BaseAddress !=3D NULL) && (RegionLength !=3D NULL) && (RegionAt= tributes !=3D NULL));=0D + if ((BaseAddress =3D=3D NULL) || (RegionLength =3D=3D NULL) || (RegionAt= tributes =3D=3D NULL)) {=0D + ASSERT ((BaseAddress !=3D NULL) && (RegionLength !=3D NULL) && (Region= Attributes !=3D NULL));=0D + return EFI_INVALID_PARAMETER;=0D + }=0D =0D TranslationTable =3D ArmGetTTBR0BaseAddress ();=0D =0D diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mm= u.c=0D index 268c0bf3f9ae..5a2f36d06086 100644=0D --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c=0D +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c=0D @@ -217,7 +217,10 @@ SyncCacheConfigPage (=0D } else if (PageAttributes !=3D NextPageAttributes) {=0D // Convert Section Attributes into GCD Attributes=0D Status =3D PageToGcdAttributes (NextPageAttributes, &GcdAttributes= );=0D - ASSERT_EFI_ERROR (Status);=0D + if (EFI_ERROR (Status)) {=0D + ASSERT_EFI_ERROR (Status);=0D + GcdAttributes =3D 0;=0D + }=0D =0D // update GCD with these changes (this will recurse into our own C= puSetMemoryAttributes below which is OK)=0D SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, = *NextRegionBase, *NextRegionLength, GcdAttributes);=0D @@ -230,7 +233,10 @@ SyncCacheConfigPage (=0D } else if (NextPageAttributes !=3D 0) {=0D // Convert Page Attributes into GCD Attributes=0D Status =3D PageToGcdAttributes (NextPageAttributes, &GcdAttributes);= =0D - ASSERT_EFI_ERROR (Status);=0D + if (EFI_ERROR (Status)) {=0D + ASSERT_EFI_ERROR (Status);=0D + GcdAttributes =3D 0;=0D + }=0D =0D // update GCD with these changes (this will recurse into our own Cpu= SetMemoryAttributes below which is OK)=0D SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, *N= extRegionBase, *NextRegionLength, GcdAttributes);=0D @@ -278,7 +284,12 @@ SyncCacheConfig (=0D //=0D MemorySpaceMap =3D NULL;=0D Status =3D gDS->GetMemorySpaceMap (&NumberOfDescriptors, &Memory= SpaceMap);=0D - ASSERT_EFI_ERROR (Status);=0D +=0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - GetMemorySpaceMap() failed! St= atus: %r\n", Status));=0D + ASSERT_EFI_ERROR (Status);=0D + return Status;=0D + }=0D =0D // The GCD implementation maintains its own copy of the state of memory = space attributes. GCD needs=0D // to know what the initial memory space attributes are. The CPU Arch. = Protocol does not provide a=0D @@ -307,7 +318,12 @@ SyncCacheConfig (=0D } else if (SectionAttributes !=3D NextSectionAttributes) {=0D // Convert Section Attributes into GCD Attributes=0D Status =3D SectionToGcdAttributes (NextSectionAttributes, &GcdAttr= ibutes);=0D - ASSERT_EFI_ERROR (Status);=0D +=0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes()= failed! Status: %r\n", Status));=0D + ASSERT_EFI_ERROR (Status);=0D + GcdAttributes =3D 0;=0D + }=0D =0D // update GCD with these changes (this will recurse into our own C= puSetMemoryAttributes below which is OK)=0D SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, = NextRegionBase, NextRegionLength, GcdAttributes);=0D @@ -343,7 +359,11 @@ SyncCacheConfig (=0D if (NextSectionAttributes !=3D 0) {=0D // Convert Section Attributes into GCD Attributes=0D Status =3D SectionToGcdAttributes (NextSectionAttributes, &GcdAttr= ibutes);=0D - ASSERT_EFI_ERROR (Status);=0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes()= failed! Status: %r\n", Status));=0D + ASSERT_EFI_ERROR (Status);=0D + GcdAttributes =3D 0;=0D + }=0D =0D // update GCD with these changes (this will recurse into our own C= puSetMemoryAttributes below which is OK)=0D SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, = NextRegionBase, NextRegionLength, GcdAttributes);=0D @@ -360,7 +380,11 @@ SyncCacheConfig (=0D if (NextSectionAttributes !=3D 0) {=0D // Convert Section Attributes into GCD Attributes=0D Status =3D SectionToGcdAttributes (NextSectionAttributes, &GcdAttribut= es);=0D - ASSERT_EFI_ERROR (Status);=0D + if (EFI_ERROR (Status)) {=0D + DEBUG ((DEBUG_ERROR, "SyncCacheConfig - SectionToGcdAttributes() fai= led! Status: %r\n", Status));=0D + ASSERT_EFI_ERROR (Status);=0D + GcdAttributes =3D 0;=0D + }=0D =0D // update GCD with these changes (this will recurse into our own CpuSe= tMemoryAttributes below which is OK)=0D SetGcdMemorySpaceAttributes (MemorySpaceMap, NumberOfDescriptors, Next= RegionBase, NextRegionLength, GcdAttributes);=0D -- =0D 2.41.0.windows.1=0D =0D