From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web10.79444.1675761402301415445 for ; Tue, 07 Feb 2023 01:16:42 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=me/eEQE3; spf=pass (domain: kernel.org, ip: 145.40.68.75, 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 ams.source.kernel.org (Postfix) with ESMTPS id 3869CB8184D for ; Tue, 7 Feb 2023 09:16:40 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id DC2DCC433D2 for ; Tue, 7 Feb 2023 09:16:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1675761398; bh=lHP9FZhtLY+jv1DJIC8ewoUef5H/IpiluiJqz9RBEtw=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=me/eEQE3edcGCFQiKzxi1W9p1qBf4AnDD4eQmO0ZgmjnW21N/faSVMd80NWY8i246 7bEfGrwj7U1W03HNHnSpbRE+dwzz8cjL4E+AJGClCLMz8EvpqO2IKlYH1VsMDWiU2l ZeIppd/W4ImbX+OugOFWSFZsgmlXE7Z9lErKJroK0DmY0/IbmahkyiFJ9YCeswJzkF RRBlKfqUdiXqPdfwRViwQUB8+j2UpLe5LFNo6VILnFosFP7X3YYeZuGiO0BMRMsW5G inKsmxDcmiIjB13bqU/HlkAX+ajsg5MdbOmtv/RRR34zrHJEYLKD5GOj/fzoKLn6gp hzpnkr1Gw52mQ== Received: by mail-lj1-f172.google.com with SMTP id b13so14807681ljf.8 for ; Tue, 07 Feb 2023 01:16:38 -0800 (PST) X-Gm-Message-State: AO0yUKVyYe56bwbu2b0IHoFJz6S9pQwg46c5mUrhwxXT7+kpprweIffz z2TH9ricXxeBMy+pvSVGIpDc2ZGC+pfwhCXUJzg= X-Google-Smtp-Source: AK7set9zxmYzs9fmbEJToZ8KiHkCrmGpVcZ5YCkeHnoicPrqYM2bBhihggvPEmlAZ8VZsVj5hG8w1laCUWpRwbZHTNw= X-Received: by 2002:a2e:8206:0:b0:290:5b9d:e97 with SMTP id w6-20020a2e8206000000b002905b9d0e97mr352251ljg.187.1675761396890; Tue, 07 Feb 2023 01:16:36 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: "Ard Biesheuvel" Date: Tue, 7 Feb 2023 10:16:25 +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 09:56, Marvin H=C3=A4user wrote: > > 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 Bugzil= la > >> 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 looping. > >> > > > > 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 changing > > 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 und= erstood Taylor correctly. The only scenario I first-hand experienced this b= ug with was unloading a PE image. I don=E2=80=99t have the time right now t= o check the guarding page code in detail, but from what I just saw, I can v= ery well imagine it can trigger the BZ bug (and thus potentially the recurs= ion 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. > > > > > > >> I implemented pre-allocated pages for ARM a while back in a private re= po > >> but never committed it to Mu. Maybe that would also be worth committin= g > >> and pushing upstream. > >> > > > > I'd like to understand better whether or not there is a way to avoid > > the need for this, but if not, I'd be happy to review your solution. > > Does the issue only exist on ARM? Does it still happen after I rewrote > > the MMU library? (in 2020) > > Sorry to interject with no contribution, but for x86 platforms, our downs= tream fork removed the requirement that BSData and ConvMem need to have the= same permissions. In fact, ideally ConvMem is just unmapped. Can this be e= nabled for ARM without a solution like Taylor=E2=80=99s? You said you added= the requirement as a mitigation. > Mapping a single page in a large unmapped region may require the allocation of page tables. If populating that page table means we have to map it first, we have a circular dependency and the recursion, so we cannot deal with that currently. Note that the page table walker does not need this page to be mapped, it is only the code running on the CPU that needs a mapping while it creates the entries. So this is fixable in principle, by mapping the page somewhere else in the address space temporarily, just so we can populate its contents without the need for recursing into the page table code to map the page table. This would take a bit of work, though, and I'd like to avoid this if possib= le. > Unrelated FYI, we also removed the XP checks for code downstream and all = types but ConvMem (which is unmapped or read-only, I forgot) have default p= ermissions of RW. The reason for that is that unlike an OS, we don=E2=80=99= t have a fully-featured VM system and especially things like mmap are absen= t. Thus, any data or code must first be written to the memory before it can= be executed. The execute flag is added after loading the code to ensure W^= X. > I would like to do the same, but this is currently not feasible. I recently removed the exec permissions from EfiLoaderData regions in ArmVirtQemu, and even though GRUB was fixed 5+ years ago not to rely on this, a couple of distro forks got broken, and so they had to revert this change in their builds. With the EFI memory attributes protocol, the OS can tolerate this, and I am adding handling of this to Linux so it no longer relies on any allocation being executable by default.