From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail05.groups.io (mail05.groups.io [45.79.224.7]) by spool.mail.gandi.net (Postfix) with ESMTPS id 8F58EAC1961 for ; Fri, 10 Jan 2025 17:25:41 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=+tcAwtl/DPXRzHszZd6bTfh06YQkEULUavZxah65JvE=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240830; t=1736529941; v=1; x=1736789140; b=R52aXTJdXDAijoH+H6/bPzx+jsbvBAhMejTyXcym53IKRYM1arL2Oml2gFJ3jrUIeOwwQu4O MSowfmLJfrhOq6OuOKE2vX6fUEJ8IYidRNBICoUdiaV1mV1zf1g+GXfPhWH4wIgDqx4DEm1il3r T50iGaco630n7VQ08GIE+e+Md0KzmNnATv/5kQj0JHLtJaxQHg+71jtuOk6Sbr8bQ7nTk7Cf4vX h0ouuTMShiY6jvDqfLQh1mNsEdRkcuS1cXb7qhvYD4smLMle/puEFvo1iIW4pcoDxOXRGIbwlfo 4yhzMeBKYG6fVincvQkhEge32a0ixyFNNKn74Qf5xCl4g== X-Received: by 127.0.0.2 with SMTP id skF3YY7687511xVF8EcyBSoc; Fri, 10 Jan 2025 09:25:40 -0800 X-Received: from nyc.source.kernel.org (nyc.source.kernel.org [147.75.193.91]) by mx.groups.io with SMTP id smtpd.web11.24948.1736529939027160567 for ; Fri, 10 Jan 2025 09:25:39 -0800 X-Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by nyc.source.kernel.org (Postfix) with ESMTP id 84DDDA42754 for ; Fri, 10 Jan 2025 17:23:49 +0000 (UTC) X-Received: by smtp.kernel.org (Postfix) with ESMTPSA id A518AC4CEE4 for ; Fri, 10 Jan 2025 17:25:37 +0000 (UTC) X-Received: by mail-lj1-f179.google.com with SMTP id 38308e7fff4ca-3043e84c687so17716231fa.1 for ; Fri, 10 Jan 2025 09:25:37 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCVFgtf/KpOr2oo04CaG2sl2qc9/9EAbudRstTk1+CXsP/qMuCBvronVZLQU99E9zFzj2yrC4Q==@edk2.groups.io X-Gm-Message-State: lrgP4xBcmxlgmQY3WPLNEwfWx7686176AA= X-Google-Smtp-Source: AGHT+IH1YfVBqbm+Pgf7TBeHFLTJOJ9Hm6qsUhBbmahKzi8ZRlMk7bsMFmjtEcNd8F2vUCVRY9HYSEle6Ia2vdTZc80= X-Received: by 2002:a05:651c:1415:b0:304:9de0:7d9 with SMTP id 38308e7fff4ca-305f45a0f1fmr28575561fa.21.1736529935923; Fri, 10 Jan 2025 09:25:35 -0800 (PST) MIME-Version: 1.0 References: <20250108215957.3437660-1-usamaarif642@gmail.com> <20250108215957.3437660-2-usamaarif642@gmail.com> <8613563a-ee7c-4271-b1f0-4d1ac365ad3a@gmail.com> In-Reply-To: <8613563a-ee7c-4271-b1f0-4d1ac365ad3a@gmail.com> From: "Ard Biesheuvel via groups.io" Date: Fri, 10 Jan 2025 18:25:24 +0100 X-Gmail-Original-Message-ID: X-Gm-Features: AbW1kvYPA0MmDvM52yYY8OWWdO9cYIpDAVPSEFxEiFLV6IJYQpauDzlmq49gb3Q Message-ID: Subject: Re: [edk2-devel] [RFC 1/2] efi/memattr: Use desc_size instead of total size to check for corruption To: Usama Arif Cc: linux-efi@vger.kernel.org, devel@edk2.groups.io, kexec@lists.infradead.org, hannes@cmpxchg.org, dyoung@redhat.com, x86@kernel.org, linux-kernel@vger.kernel.org, leitao@debian.org, gourry@gourry.net, kernel-team@meta.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Fri, 10 Jan 2025 09:25:39 -0800 Resent-From: ardb@kernel.org Reply-To: devel@edk2.groups.io,ardb@kernel.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: text/plain; charset="UTF-8" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=R52aXTJd; dmarc=pass (policy=none) header.from=groups.io; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io On Fri, 10 Jan 2025 at 11:53, Usama Arif wrote: > > > > On 10/01/2025 07:21, Ard Biesheuvel wrote: > > On Thu, 9 Jan 2025 at 17:36, Usama Arif wrote: > >> > >> > >> > >> On 09/01/2025 15:45, Ard Biesheuvel wrote: > >>> On Wed, 8 Jan 2025 at 23:00, Usama Arif wrote: > >>>> > >>>> The commit in [1] introduced a check to see if EFI memory attributes > >>>> table was corrupted. It assumed that efi.memmap.nr_map remains > >>>> constant, but it changes during late boot. > >>>> Hence, the check is valid during cold boot, but not in the subsequent > >>>> kexec boot. > >>>> > >>>> This is best explained with an exampled. At cold boot, for a test > >>>> machine: > >>>> efi.memmap.nr_map=91, > >>>> memory_attributes_table->num_entries=48, > >>>> desc_size = 48 > >>>> Hence, the check introduced in [1] where 3x the size of the > >>>> entire EFI memory map is a reasonable upper bound for the size of this > >>>> table is valid. > >>>> > >>>> In late boot __efi_enter_virtual_mode calls 2 functions that updates > >>>> efi.memmap.nr_map: > >>>> - efi_map_regions which reduces the `count` of map entries > >>>> (for e.g. if should_map_region returns false) and which is reflected > >>>> in efi.memmap by __efi_memmap_init. > >>>> At this point efi.memmap.nr_map becomes 46 in the test machine. > >>>> - efi_free_boot_services which also reduces the number of memory regions > >>>> available (for e.g. if md->type or md->attribute is not the right value). > >>>> At this point efi.memmap.nr_map becomes 9 in the test machine. > >>>> Hence when you kexec into a new kernel and pass efi.memmap, the > >>>> paramaters that are compared are: > >>>> efi.memmap.nr_map=9, > >>>> memory_attributes_table->num_entries=48, > >>>> desc_size = 48 > >>>> where the check in [1] is no longer valid with such a low efi.memmap.nr_map > >>>> as it was reduced due to efi_map_regions and efi_free_boot_services. > >>>> > >>>> A more appropriate check is to see if the description size reported by > >>>> efi and memory attributes table is the same. > >>>> > >>>> [1] https://lore.kernel.org/all/20241031175822.2952471-2-ardb+git@google.com/ > >>>> > >>>> Fixes: 8fbe4c49c0cc ("efi/memattr: Ignore table if the size is clearly bogus") > >>>> Reported-by: Breno Leitao > >>>> Signed-off-by: Usama Arif > >>>> --- > >>>> drivers/firmware/efi/memattr.c | 16 ++++++---------- > >>>> 1 file changed, 6 insertions(+), 10 deletions(-) > >>>> > >>> > >>> The more I think about this, the more I feel that kexec on x86 should > >>> simply discard this table, and run with the firmware code RWX (which > >>> is not the end of the world). > >> > >> > >> By discard this table, do you mean kexec not use e820_table_firmware? > > > > No, I mean kexec ignores the memory attributes table. > > > >> Also a very basic question, what do you mean by run with the firmware RWX? > >> > > > > The memory attributes table is an overlay for the EFI memory map that > > describes which runtime code regions may be mapped with restricted > > permissions. Without this table, everything will be mapped writable as > > well as executable, but only in the EFI page tables, which are only > > active when an EFI call is in progress. > > > > Thanks for explaining! > > So basically get rid of memattr.c :) > > Do you mean get rid of it only for kexec, or not do it for any > boot (including cold boot)? Only for kexec, and only on x86 > I do like this idea! I couldn't find this in the git history, > but do you know if this was added in the linux kernel just > because EFI spec added support for it, or if there was a > specific security problem? > Mapping memory RWX is generally a bad idea, so we should avoid it if possible. But EFI runtime memory regions are only mapped during a EFI runtime call, and these don't happen often at all, so the benefit is only marginal. (In the early days of EFI, it was more common for the OS to map these regions permanently, but we stopped doing that a long time ago) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#120980): https://edk2.groups.io/g/devel/message/120980 Mute This Topic: https://groups.io/mt/110517813/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-