From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web12.33955.1585218165909829032 for ; Thu, 26 Mar 2020 03:22:46 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=Mj3grCli; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id m11so1128162wrx.10 for ; Thu, 26 Mar 2020 03:22:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=cCj/ykAWkPcivjmVDCKEoJPTszHx16qEzQZiY6nLGEk=; b=Mj3grClitJODNt2H0HVhVn3iG8xvXcLmzUs2NRnRNiHRlyhv7+0tY6gvcYvlY2ouhf 6aG4Qd6WYzmeVrizp0+qDKfEczgr7EsyjRoHucpl9TMBfKJqto4QOXb7wnhHSMc9r6XX mGtoAUZ9TRhfSEhz/O9ySODodssw9d2NxIs3W1o2W3z+c3TbnbqBxraQPTSXBqhiWQfj GQ6+1ZMIEtiwNGY38iGFcpVXKm/WPqLIJ6DzXPsBQlr7rKLLwgAkgopB2G3GgmiYcADF 79GSflT43qTjlSmADCxsMUDu9pMVpbzBFueUpgc22MFxDoCNoUoD1/xQPGpuszU+Bb+K F2kA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=cCj/ykAWkPcivjmVDCKEoJPTszHx16qEzQZiY6nLGEk=; b=LOdX+PYQbhe/2f+ynjZoyYh47VbsoMSHyGqUgh+Ecu8O6pKzNXZ0fpNwz9j/2pjS6G i3mt0DVXi4GSfs57/UQj8YukSOdVhcWj1q/RGH5dHtiF4s/ojExhyyUZxI6Cd8W6hPSP +7nl60iVZW5xSMcSgQXD5GcIY6Aq9UAHzKfnHPB6JMqcFFU/yqKI/guyRyEKd6NCr/Z0 maM3LFYO+wMS5kS6lYTEMk5oxTV3V/qo8ORSw5CLD6iKquDTDLcPZKK393EjSkfuSMB8 0P4skHazW6tDoQaCJQ/l5xlD8AI6mGMcjxbATP/cAA4CEpha42SWpFYhQwAnZ8KVyQP4 VmBg== X-Gm-Message-State: ANhLgQ2CmtdRx4encHEzRoICDl9rfX0LBfDvqfoyxLqNyvX+VBcAGTlm KVjJ8Ted8LSZvSkSc/w2WXQAYg== X-Google-Smtp-Source: ADFU+vs2VLEP16HcSn3woa0NuzGNFFupqyCqB3D5kyBIR00saaYA9ph46eohN633IyXSEulCn1ZxKA== X-Received: by 2002:adf:f58c:: with SMTP id f12mr9073073wro.207.1585218164471; Thu, 26 Mar 2020 03:22:44 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id 71sm3123035wrc.53.2020.03.26.03.22.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Mar 2020 03:22:43 -0700 (PDT) Date: Thu, 26 Mar 2020 10:22:42 +0000 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, Laszlo Ersek , Ashish Singhal Subject: Re: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables Message-ID: <20200326102242.GO22097@bivouac.eciton.net> References: <20200325152940.1492-1-ard.biesheuvel@linaro.org> <20200325152940.1492-2-ard.biesheuvel@linaro.org> MIME-Version: 1.0 In-Reply-To: <20200325152940.1492-2-ard.biesheuvel@linaro.org> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Mar 25, 2020 at 16:29:38 +0100, Ard Biesheuvel wrote: > FreePageTablesRecursive () traverses the page table tree depth first > to free all pages that it finds, without taking into account the > level at which it is operating. > > Since TT_TYPE_TABLE_ENTRY aliases TT_TYPE_BLOCK_ENTRY_LEVEL3, we cannot > distinguish table entries from block entries unless we take the level > into account, and so we may be dereferencing garbage if we happen to > try and free a hierarchy of page tables that has level 3 pages in it. > > Let's fix this by passing the level into FreePageTablesRecursive (), > and limit the recursion to levels < 3. > > Signed-off-by: Ard Biesheuvel LGTM Reviewed-by: Leif Lindholm One question, which does not require any action on this patch: Could that 3 be a #define (or Pcd) for future-proofing, or do the bindings already restrict us in ways that can't reasonably be tweaked? > --- > ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > index a43d468b73ca..d78918cf7ba8 100644 > --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c > @@ -142,15 +142,21 @@ ReplaceTableEntry ( > STATIC > VOID > FreePageTablesRecursive ( > - IN UINT64 *TranslationTable > + IN UINT64 *TranslationTable, > + IN UINTN Level > ) > { > UINTN Index; > > - for (Index = 0; Index < TT_ENTRY_COUNT; Index++) { > - if ((TranslationTable[Index] & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) { > - FreePageTablesRecursive ((VOID *)(UINTN)(TranslationTable[Index] & > - TT_ADDRESS_MASK_BLOCK_ENTRY)); > + ASSERT (Level <= 3); > + > + if (Level < 3) { > + for (Index = 0; Index < TT_ENTRY_COUNT; Index++) { > + if ((TranslationTable[Index] & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) { > + FreePageTablesRecursive ((VOID *)(UINTN)(TranslationTable[Index] & > + TT_ADDRESS_MASK_BLOCK_ENTRY), > + Level + 1); > + } > } > } > FreePages (TranslationTable, 1); > @@ -254,7 +260,7 @@ UpdateRegionMappingRecursive ( > // possible for existing table entries, since we cannot revert the > // modifications we made to the subhierarchy it represents.) > // > - FreePageTablesRecursive (TranslationTable); > + FreePageTablesRecursive (TranslationTable, Level + 1); > } > return Status; > } > -- > 2.17.1 >