From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.39.1594229753620653154 for ; Wed, 08 Jul 2020 10:35:53 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=EFl6GQtv; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594229752; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=WiJBmMH6MOhvjBR/i/xpPUwzlAqEmcGuhGUse57MrKU=; b=EFl6GQtv1Xu96LgsjtS+H9J96rPCvPO2SEHk4WVjOwfbDO7YB05nk6JYWIjpvXbNZ3UgJP 3bMlkein55sYvpBp3Zjc1T2TLuxUHllUBjUg2rLXYwmrYR1cfItDJ8eEmpECyNL8wweukt iqqD2rQTkOEiAS8UBjuJ4Gb68ceO0Bc= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-335-tnVBvpI7PTW-oHjXKXvrPQ-1; Wed, 08 Jul 2020 13:35:38 -0400 X-MC-Unique: tnVBvpI7PTW-oHjXKXvrPQ-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 94B1919253CA; Wed, 8 Jul 2020 17:35:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-73.ams2.redhat.com [10.36.112.73]) by smtp.corp.redhat.com (Postfix) with ESMTP id A790E724A2; Wed, 8 Jul 2020 17:35:34 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 4/9] UefiCpuPkg/SecMigrationPei: Add initial PEIM (CVE-2019-11098) To: devel@edk2.groups.io, guomin.jiang@intel.com Cc: Michael Kubacki , Eric Dong , Ray Ni , Rahul Kumar , Debkumar De , Harry Han , Catharine West References: <20200708081059.691-1-guomin.jiang@intel.com> <20200708081059.691-5-guomin.jiang@intel.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 8 Jul 2020 19:35:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200708081059.691-5-guomin.jiang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/08/20 10:10, Guomin Jiang wrote: > From: Michael Kubacki > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1614 > > Adds a PEIM that republishes structures produced in SEC. This > is done because SEC modules may not be shadowed in some platforms > due to space constraints or special alignment requirements. The > SecMigrationPei module locates interfaces that may be published in > SEC and reinstalls the interface with permanent memory addresses. > > This is important if pre-memory address access is forbidden after > memory initialization and data such as a PPI descriptor, PPI GUID, > or PPI inteface reside in pre-memory. [...] > +EFI_STATUS > +EFIAPI > +SecMigrationPeiInitialize ( > + IN EFI_PEI_FILE_HANDLE FileHandle, > + IN CONST EFI_PEI_SERVICES **PeiServices > + ) > +{ > + EFI_STATUS Status = EFI_SUCCESS; > + > + if (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes)) { > + Status = PeiServicesInstallPpi (&mEdkiiRepublishSecPpiDescriptor); > + ASSERT_EFI_ERROR (Status); > + } > + > + return Status; > +} Thank you for squashing the PCD check into the patch that introduces the PEIM. I have two superficial comments: (a) In the edk2 coding style, we don't initialize local variables (meaning "initialization" in the strict C language sense). That is, EFI_STATUS Status = EFI_whatever; should be replaced with EFI_STATUS Status; Status = EFI_whatever; (b) If the PCD is false (and so we don't install the PPI), then keeping the PEIM loaded seems useless. When a module determines that it should not stay resident (because it does not expose services to other modules), it usually returns EFI_ABORTED or something similar from the entry point function. Then the PEI or DXE (or SMM) core unloads the module at once. Therefore I think it would make sense to set Status to EFI_ABORTED, before the PCD check. With (a) and (b) updated: Acked-by: Laszlo Ersek Thanks Laszlo