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 E8099D8114D for ; Wed, 15 Jan 2025 18:53:14 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=LRWQBxuotilAecAVth7WZR9XhixMzW9DuwMdonfJZvk=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20240830; t=1736967194; v=1; x=1737226393; b=Jo62+JgNvq+v/VwxPa97xkGLn8QsoJE8FFqH8ubC8Tmyp4TV9k1JIK8d5ljam6dOwjQnxQ0p yYhtrBXQzE1FNOZHdeXHYNlwA6hGkwuhQv8nos+/ER2scvYEcbSrddEPQ9+nbUajgXWO46Bo9VG gAaeWUkvY9gkA/PuLuNPFh5b8IYgfTl6MqxTLdR94FryAujPRksQPXfhTiGsh/VZSu6QTqQOtOW 5Q4yokx2KTJD8qAcUt0tjBqoKbJosE0AdDoRbWVT+8QYGuq0gppQpVpERUPAGjaIuPoqCOJxXqe Yisa3JooPnuxjDuaiac45aoj8ak1HPU7Uc0uFW1WNWN5g== X-Received: by 127.0.0.2 with SMTP id 1OyNYY7687511xSH91WSqvOl; Wed, 15 Jan 2025 10:53:13 -0800 X-Received: from mail-ed1-f53.google.com (mail-ed1-f53.google.com [209.85.208.53]) by mx.groups.io with SMTP id smtpd.web10.14763.1736767659649890386 for ; Mon, 13 Jan 2025 03:27:39 -0800 X-Received: by mail-ed1-f53.google.com with SMTP id 4fb4d7f45d1cf-5d7e3f1fdafso8522084a12.0 for ; Mon, 13 Jan 2025 03:27:39 -0800 (PST) X-Forwarded-Encrypted: i=1; AJvYcCUk5tQdVQ0wGFrQdKJCR5qfV8ls+JnlFtUothrZl/Q/A3Wn6kDCLVCVw1gSAVJp5rynBR+pJw==@edk2.groups.io X-Gm-Message-State: 4Z1jsqOewuaTsomPNEOFVLCvx7686176AA= X-Gm-Gg: ASbGncsYBtYLHFE3+mF60wDtl+KYzvdSndUPjGDXvfsb2RFLryizcsVrU1tYroU4Ym7 zdC9oFNsGf/nthUGLIiF02Yj6klHsqmDxut2ApgdZ4izMIpuvO3LIUNq5NWGsj33OwTgiBjgGu4 UEURpS0YIu0fWBUeOsVdI3xZWZ4YRtf+MYLuDfGtllvJIzgEM8b3cnM5kX3VgsflKXG1UNTOxIb MMUJAUqbNAjO38bxAczVNEMGGE83BU3/EWsI4EFgBKNq5iI0JYSJ+YA3zo9NmEYGl1/Hr9/UWbT Q6JAaqODs2X1BzE7Ds7DUeTMYjWJKvZiEA== X-Google-Smtp-Source: AGHT+IHY6ghLf4F1/E7hRXTvtL58y1ipNyvjXd5BzF87rdGENcFmDQO+YbClGjqRU3qWE4NMAa/7Lw== X-Received: by 2002:a05:6402:5245:b0:5d0:ee52:353e with SMTP id 4fb4d7f45d1cf-5d972e696a4mr19574109a12.29.1736767657514; Mon, 13 Jan 2025 03:27:37 -0800 (PST) X-Received: from ?IPV6:2a03:83e0:1126:4:829:739b:3caa:6500? ([2620:10d:c092:500::6:97ef]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-5d9904a55f9sm4673231a12.81.2025.01.13.03.27.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 13 Jan 2025 03:27:37 -0800 (PST) Message-ID: Date: Mon, 13 Jan 2025 11:27:36 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [RFC 1/2] efi/memattr: Use desc_size instead of total size to check for corruption To: Dave Young Cc: Ard Biesheuvel , linux-efi@vger.kernel.org, devel@edk2.groups.io, kexec@lists.infradead.org, hannes@cmpxchg.org, x86@kernel.org, linux-kernel@vger.kernel.org, leitao@debian.org, gourry@gourry.net, kernel-team@meta.com References: <20250108215957.3437660-1-usamaarif642@gmail.com> <20250108215957.3437660-2-usamaarif642@gmail.com> <8613563a-ee7c-4271-b1f0-4d1ac365ad3a@gmail.com> From: "Usama Arif via groups.io" In-Reply-To: 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: Wed, 15 Jan 2025 10:53:11 -0800 Resent-From: usamaarif642@gmail.com Reply-To: devel@edk2.groups.io,usamaarif642@gmail.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=Jo62+JgN; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 45.79.224.7 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=pass (policy=none) header.from=groups.io On 13/01/2025 02:33, Dave Young wrote: > On Fri, 10 Jan 2025 at 18:54, 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 wrot= e: >>>>>> >>>>>> 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 subsequen= t >>>>>> kexec boot. >>>>>> >>>>>> This is best explained with an exampled. At cold boot, for a test >>>>>> machine: >>>>>> efi.memmap.nr_map=3D91, >>>>>> memory_attributes_table->num_entries=3D48, >>>>>> desc_size =3D 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 th= is >>>>>> 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 reflect= ed >>>>>> 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 reg= ions >>>>>> 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=3D9, >>>>>> memory_attributes_table->num_entries=3D48, >>>>>> desc_size =3D 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@go= ogle.com/ >>>>>> >>>>>> Fixes: 8fbe4c49c0cc ("efi/memattr: Ignore table if the size is clear= ly 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)? >> 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? >> >=20 > Usama, can you try the patch below? > [ format is wrong due to webmail corruption. But if it works I can > send a formal patch later ] >=20 > $ git diff arch/x86 > diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirk= s.c > index 846bf49f2508..58dc77c5210e 100644 > --- a/arch/x86/platform/efi/quirks.c > +++ b/arch/x86/platform/efi/quirks.c > @@ -561,6 +561,11 @@ int __init efi_reuse_config(u64 tables, int nr_table= s) >=20 > if (!efi_guidcmp(guid, SMBIOS_TABLE_GUID)) > ((efi_config_table_64_t *)p)->table =3D data->smb= ios; > + > + /* Not bother to play with mem attr table across kexec */ > + if (!efi_guidcmp(guid, EFI_MEMORY_ATTRIBUTES_TABLE_GUID)) > + ((efi_config_table_64_t *)p)->table =3D > EFI_INVALID_TABLE_ADDR; > + > p +=3D sz; > } >=20 This would work, I am guessing it will have a similar effect to what I sent last week in=20 https://lore.kernel.org/all/fd63613c-fd26-42de-b5ed-cc734f72eb36@gmail.com/ I think it needs to be wrapped in ifdef CONFIG_X86_64. -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#121017): https://edk2.groups.io/g/devel/message/121017 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-