From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web10.79946.1675764076080103569 for ; Tue, 07 Feb 2023 02:01:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=KRZ8g+6q; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 48CC3612AC for ; Tue, 7 Feb 2023 10:01:15 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id AC81BC433EF for ; Tue, 7 Feb 2023 10:01:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675764074; bh=0JG7gi+3pqlO4WEf1Uzz+RROVY4dKaZvxMtEeuIS/d8=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=KRZ8g+6q7dzUv6zf3vbbp/yQEcTBgW1Y9TI4Sw6Tsm4+a0ZN55vV7paQpaYZhTlRl Fdc+Ix0pRTZs+W6dQdMqRjbN+cIIeC37gzMjJW8wPBjdvpiu9L/z/k1vFqNtmYqC8o 9Tzo+R8ORu26K1vMwieStDt61Pc8riISzeYqmdN94eZziSkLQqAHxnVbB0Us0QT5Qe etJSopTP9+10xJ/qGp52SYNmTmALXbFp15NV5PkS5/K0dCZ0W0csx6Hctp9xcUi9nB c17OzfEIGgf9M31/mlT1FfZPTM3+DC87buy/EujmYOyuqRFgLy4i1NNUfRRjZvhwi8 lRrQPZeuZFBOA== Received: by mail-lf1-f54.google.com with SMTP id h24so21606459lfv.6 for ; Tue, 07 Feb 2023 02:01:14 -0800 (PST) X-Gm-Message-State: AO0yUKXfjUO5FGRcVaKkcJbY/leOfF9N+I9tG7r1VvMRmivcIfOvuElx k5e9mUL5gSaSSULRwM08hz0t0OBQfFB1/57lLBY= X-Google-Smtp-Source: AK7set8S0qRtVLXmkgMwv6Ln9nF+uR8yjmBEUVaEGwxN4OrXMdTiY1c5AcoBtsJzAfDt5c/epP8umagetRtA994cwZY= X-Received: by 2002:ac2:5550:0:b0:4b6:e197:3aeb with SMTP id l16-20020ac25550000000b004b6e1973aebmr399606lfk.233.1675764072644; Tue, 07 Feb 2023 02:01:12 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 7 Feb 2023 11:01:01 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH 4/4] ArmPkg/CpuDxe: Implement EFI memory attributes protocol To: devel@edk2.groups.io, mhaeuser@posteo.de Cc: t@taylorbeebe.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 7 Feb 2023 at 10:16, Ard Biesheuvel wrote: > > On Tue, 7 Feb 2023 at 09:56, Marvin H=C3=A4user wrot= e: > > > > Hi Taylor and Ard, > > > > > On 7. Feb 2023, at 09:29, Ard Biesheuvel wrote: > > > > > > =EF=BB=BFOn Tue, 7 Feb 2023 at 02:18, Taylor Beebe wrote: > > >> > > >> I can't see the Bugzilla you referenced so I requested security Bugz= illa > > >> access. But, yes, that's the bug to which I was referring :) > > >> > > > > > > I cannot see that bugzilla entry either. > > > > I CC=E2=80=99d you both. > > > > Thanks. > > I wrote that code but nobody ever involved me or mentioned that there > was anything wrong with it. > > > > > > >> Once Ard's change to add Memory Attribute Protocol support to ARM > > >> platforms is in, the change you linked may be palatable for the > > >> upstream. However, ARM platforms could run into the infinite loop I > > >> outlined if that change is pushed upstream because of the lack of > > >> per-allocated page tables and a software semaphore to prevent loopin= g. > > >> > > > > > > I still don't see how this happens. There is an ASSERT in > > > CoreInitializeMemoryProtection() to ensure that EfiConventialMemory > > > and EfiBootServicesData have the same memory type, and I added that > > > (in commit 7eb927db3e25a) for precisely this reason, i.e., to ensure > > > that the plumbing of this feature wouldn't recurse. > > > > > > Could this be related to heap guard, perhaps? I could see how changin= g > > > the boundaries of allocations might trigger a split even if the old > > > and new type should have the exact same type, and perhaps we should > > > use unguarded pages for page tables. > > > > I know you meant recursing, but that might be related to the BZ, if I u= nderstood Taylor correctly. The only scenario I first-hand experienced this= bug with was unloading a PE image. I don=E2=80=99t have the time right now= to check the guarding page code in detail, but from what I just saw, I can= very well imagine it can trigger the BZ bug (and thus potentially the recu= rsion issue?). > > if we disregard explicit invocations of > EFI_DXE_SERVICES.SetMemorySpaceAttributes (), the typical region > transitions from EfiConventionalMemory to some other type and back, > and never directly from one type to another. So I would expect that > unloading a PE image would result in a FreePages () call with > EfiConventionalMemory as the new type. > > However, it appears that UnprotectUefiImage does not restore the > region's permissions to whatever is configured as the default for the > associated memory type, and so we end up with EfiConventialMemory > regions that lack the XP attribute. > > I'll send out a separate fix for that. We can resume the discussion on > your patch on the bugzilla at the same time. > Actually, it seems UnprotectUefiImage () is corrent under the assumption that all code regions have EFI_MEMORY_XP cleared by default. However, if you redefine the policy to set EFI_MEMORY_XP on code regions by default, and only permit execution after remapping the code read-only explicitly, and only then clearing EFI_MEMORY_XP, that routine should revert the region to EFI_MEMORY_XP. But given the existing ASSERT()s on having EFI_MEMORY_XP cleared for all code regions, the code as it is currently is not incorrect.