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 4B8239411A1 for ; Sat, 19 Apr 2025 06:04:48 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=v9iGgPIDFzpVI6xVhxqia6f1DY4IgRowhFUbJtqxxNQ=; c=relaxed/simple; d=groups.io; h=From:To:CC:Subject:Thread-Topic:Thread-Index:Date:Message-ID:References:In-Reply-To:Accept-Language:msip_labels:MIME-Version: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; s=20240830; t=1745042688; v=1; x=1745301886; b=j38nOjpF4U/BsTyAXGpm/K4ZWR40spd6lU0VF4/2ewoH2C/Lh9IMvp7exa4ZmaR3XS9YOt0n LB1WiWeO6OyY7XjxZWlQJzIiRtgm5AATeRupGPH4y6S7CThh8gFBmExtBLd/uNaVV5aPECtXg6o Vc/82aAOtl13SZF+hCGj/O4q5xLJxwzadePwXI5ywekjFiK4WAuP++d3b/zepds/pS1ElIwl2Uc qP5M8FcJ0Mt9AdwfnZx2g3+dVUtq43mLj3Au3BYPkIrRvELhTuHuxA+Ct5jJE55+LQPV6A4U3lF FaWbqzy1x47cPihEZf/Ond3pbkprpTXuycoZy+KVZW83A== X-Received: by 127.0.0.2 with SMTP id 8kZnYY7687511xec4OuCj3fe; Fri, 18 Apr 2025 23:04:46 -0700 X-Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.18]) by mx.groups.io with SMTP id smtpd.web10.2409.1745042686066838099 for ; Fri, 18 Apr 2025 23:04:46 -0700 X-CSE-ConnectionGUID: 40nSTUJ8TqmfrX2qB30XMQ== X-CSE-MsgGUID: WjxUSimiQ4qNwE0G8zbUww== X-IronPort-AV: E=McAfee;i="6700,10204,11407"; a="45896619" X-IronPort-AV: E=Sophos;i="6.15,223,1739865600"; d="scan'208,217";a="45896619" X-Received: from fmviesa010.fm.intel.com ([10.60.135.150]) by fmvoesa112.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2025 23:04:45 -0700 X-CSE-ConnectionGUID: u+praY/aS6+ypyv2v5oHqQ== X-CSE-MsgGUID: 9pUdxLJQQyKUfEEhsM7y7A== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.15,223,1739865600"; d="scan'208,217";a="131825283" X-Received: from orsmsx902.amr.corp.intel.com ([10.22.229.24]) by fmviesa010.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 18 Apr 2025 23:04:45 -0700 X-Received: from ORSMSX901.amr.corp.intel.com (10.22.229.23) by ORSMSX902.amr.corp.intel.com (10.22.229.24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.14; Fri, 18 Apr 2025 23:04:45 -0700 X-Received: from orsedg603.ED.cps.intel.com (10.7.248.4) by ORSMSX901.amr.corp.intel.com (10.22.229.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.14 via Frontend Transport; Fri, 18 Apr 2025 23:04:45 -0700 X-Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.170) by edgegateway.intel.com (134.134.137.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.44; Fri, 18 Apr 2025 23:04:45 -0700 X-Received: from CO1PR11MB4929.namprd11.prod.outlook.com (2603:10b6:303:6d::19) by CY5PR11MB6258.namprd11.prod.outlook.com (2603:10b6:930:25::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8655.22; Sat, 19 Apr 2025 06:04:12 +0000 X-Received: from CO1PR11MB4929.namprd11.prod.outlook.com ([fe80::a886:6510:729d:f9d0]) by CO1PR11MB4929.namprd11.prod.outlook.com ([fe80::a886:6510:729d:f9d0%7]) with mapi id 15.20.8655.022; Sat, 19 Apr 2025 06:04:12 +0000 From: "Michael D Kinney via groups.io" To: Kun Qin , "Ni, Ray" CC: "devel@edk2.groups.io" , "Ni, Ray" , "Kinney, Michael D" Subject: Re: [edk2-devel] A bug in the SmmCommunication V3 logic Thread-Topic: A bug in the SmmCommunication V3 logic Thread-Index: AQHbsCxo5nQBH4RMRk+1qEepe02xqLOpBL4QgACO8lCAAComQIAAMy/AgAAC4LCAAAuzgIAAAzIggAABLYCAAAK58IAAejiA Date: Sat, 19 Apr 2025 06:04:12 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ActionId=7199950a-a9af-4e88-86e5-b41f15734340;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=true;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=Internal;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2025-04-18T07:21:47Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Tag=10, 3, 0, 1; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: CO1PR11MB4929:EE_|CY5PR11MB6258:EE_ x-ms-office365-filtering-correlation-id: 0e38d89e-e543-4254-0025-08dd7f0801eb x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: =?us-ascii?Q?3vADaKB+v0iJ357ytf0JzF1W1/oua6ZKOky/MSNEuoE/x7MzRMmgGo8QEe3e?= =?us-ascii?Q?nbvrdLo4T8rqo49XOHuOytY/4aeG8q7k6ZhhMQOyBmxOhyR7iw9WGdb4XwyC?= =?us-ascii?Q?QHDfykil6jV90Par6or8wOwHoeGOM/CCvgcIS91STBZn8hbEbMg3nADnAzuG?= =?us-ascii?Q?0u3jUJEjGYFymItPBar16jJCPTnkDEmT1Azoah1egRv9Xrw5PHosThoZnE1Z?= =?us-ascii?Q?Vq0Nn0tcC3xhRhutf3bdp/BJkydp6W7rINd9y+E1nYj5DgEferVrM+aguQkn?= =?us-ascii?Q?zQ6bs34AO/Q7T++uh7YCl9YDTrNvjV0Z0hG0wdMsQZfEiuJa2WaYMSXH+eJe?= =?us-ascii?Q?Aq8wnaYGVfI5FjjhZ7kQ6qRmtFRFEKpOwRWDEXIpGeP7zIDibeMrEnggpCaq?= =?us-ascii?Q?hRxE4wJM7ih3gSJNFMlDYSbDRvzJahJBE/+r7VQQVUn9V9JCT33NkGMgN0Fx?= =?us-ascii?Q?K40kogtaE4ci4fZ30WMQyQpZYEfcbhYu/MNNmQ84El6mVaYmGlILnLQRLuJ2?= =?us-ascii?Q?Ark1jP3meRKzollrK2CUnvU2M3bUbggO/ZtDBrgFt0qu8vzOpP9LqLEQMKjK?= =?us-ascii?Q?ZkkTCLBPhVjkHFW3AdSbZtdfgSal3KU6ACSPEdLhuv6skf2GgCo7sKtjFjjX?= =?us-ascii?Q?AH67vuyk6KHH1sbeHFb60lEizI1yEBi2GCxSsblaHAaHl76r8Gls9TmlhCPs?= =?us-ascii?Q?dH6SY/6PqD0bOBovdvaRUHG5BAOYdSEvay76BuTSJNhqDMeSov+S6pwLT9wO?= =?us-ascii?Q?8WHVLVQ1mKgxwuqx2mkzf94da5Y5seuEqXXofWoyyYbMqR/lHIfJpoHpYYn5?= =?us-ascii?Q?VbMiNOzhOq2a8tGRqFPLqdSdDFVcjdo0HxW+i4l5FoG4Bo/qylc6Ihb9ojSn?= =?us-ascii?Q?EEA3b8icBYbW3oR3P2qee4SFTclX9DGNOZ3F/pd8N+NMd2moYJ9t3hJa4aBb?= =?us-ascii?Q?lZ5k5qXT8cUkDbbaYm4qYFivqxf8v8GTrY+oBu5DCJbGD34zdGGL41Ky27DL?= =?us-ascii?Q?7TiwZ7sKdbQhr6j6LNeRaII3uoDRKBzrWqkqJUHUyAoQHx930mzFGQTHpjTU?= =?us-ascii?Q?ObqMAVSSfkT89ukOEZjEBOH77AjRWkVvmc5/SRHuOY94tsTBxmJgjuY5S75R?= =?us-ascii?Q?e/lKoFqj3JO4zqEdmEkhxqh9J3MN+HUUBM4QgwhnC2SyVAA0IAR6Vr5vQYXn?= =?us-ascii?Q?5vJur02buWwfIz1L7ZEA2P7n9hWq0REsPd8hC3mDL384lmx3XoX2u5O0p2z0?= =?us-ascii?Q?/BpVZaMnRtjv0MT/WBsPsC+uCM1LidJUePzKEluqaYhYWvEaXkwqvCT1Kf/I?= =?us-ascii?Q?5Y+ownESjgdCWXIrk2SM9ghhhfT8dCAEeZGx5ux08Om4rfKbZR2m+CCPadB+?= =?us-ascii?Q?MudNwk2f/xUeHtXsLJ4sOF3W3m89qA7zvrVw8y0FBDeJx+YHPT5ixsJpElYB?= =?us-ascii?Q?aHskEcDgqOYZ6+8E3yAzPkwdRgiLJhFBhXEAuG8dPIbBXC+nXImgGvwz1RRA?= =?us-ascii?Q?WfUPPsOpWYmyyRo=3D?= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?S+eouD1AuXWLLh5DACQhbAGf5RVh/IGc/q+PX3MyJuGPKLWfNX/4T7s5OiKH?= =?us-ascii?Q?jEwFi+vKph8yLKZIZAq/UmwuA02wxBUPvg1gbEfrhghmFw4Fp1307ZzaxVYp?= =?us-ascii?Q?JjHDCzd9CT4f63liZc3tGS5+DyoftO1i74zRm8x2ehKf4uN83RII+pt7Yqqc?= =?us-ascii?Q?wePdI8wgd6GJoI2JF6yQ2W6125fpZ9EmZRo+3B47GE6c2SNXKuPyEBF25zXg?= =?us-ascii?Q?z1kDLLQkl1tAM5vwyDEstmOZajddNtPKifCRR7mHTm+n45QvsZ4OZ7JW4e2A?= =?us-ascii?Q?q+LY9LktrxFXThvy2j1j0glwMV97zqjHR1FXjBMZXLH/dLRK9FaMJabyzh0Q?= =?us-ascii?Q?NWVoEdViwfBI4P31v1yhjBsxoQBFb5U9iL2/HpbmjIt2Tw7TfqGLsSCuOq6J?= =?us-ascii?Q?DMUls9bKVVVS1a0XFFmFM1/532pSqQYVEVFDbBhBQgIEILXwmY/xZs5u9MM5?= =?us-ascii?Q?ZXBuojpTsUBO6vyPLGiO9UR/GwD1cBOXyYQq2rZ31kA8agKIYctwvC+//MFc?= =?us-ascii?Q?qDlZGyxH+Ox9oAtAKSCxxJNwcAut/e8TI/efmXfGOz+rpI4g1FkkTtVvq7vP?= =?us-ascii?Q?n9EK1xhUUTR8excDKVdK6/I76RP5HUwUrKTDPVNIc5aWU4qPdw3Or9UW/9JH?= =?us-ascii?Q?M+hWXddH0DAGK+CxVfD6QF3/NCpJalGsChu63wePM5CXgjMYiGvgOj2eBQ6C?= =?us-ascii?Q?M7KL+oAMNOXwArlvy4y2HuQJoUZK4XFkcpMBOdLApGrT4fS2KBI5J1ZWPPf2?= =?us-ascii?Q?Lap1tcgdDdvMB7cnlddGFmJ33i9KTKp3eaKWGqFaS96VEAwpemRrRwf9+Bwb?= =?us-ascii?Q?JG/F0AbXay8x/z08ZOz6fgYa1aTrFzlRY5303EKeZD/3bd4ofLCzoAgXWxuk?= =?us-ascii?Q?RASWPc7+Jz8tXjcLmWz6eAPtzZ5NB42oSpl40CflHbLlVkJlDGQnDYEHULOP?= =?us-ascii?Q?Qupfih2I6PWp03wrbk7TaCZvNziwXy4cYP6IMZO079awfD2XVbqPde0wA9UM?= =?us-ascii?Q?Ykt8kndaotASD+B3AqnZfjZsjwEM3YX4g2Al5NdehQYJiFeE+ejFKKKLMPOh?= =?us-ascii?Q?P1kHPO08D+0TnFv4XyHRU+liuqvcYwaBe2k5G+M1q8KZQjJynK68NQ9SlBGw?= =?us-ascii?Q?frSggY+bx53eao6152sJbfXJQ/C132CReCI65ztRBeVFILdcIBMfjyEyig5z?= =?us-ascii?Q?V2Gc6ET0vz2IP0nfvGgtI/G0Qim5zYvTUCNJteGcvuU8Q/DdDm2YvBTYqnOr?= =?us-ascii?Q?fAF2/eivwUV+vr+tsv714wo+Nju3zwEK7Uw3A55gF2ippcteAHwm29hxFqOy?= =?us-ascii?Q?v2GttNXgqIhc8KXPhyGobrNSTxY5Buf9PlRfLXKKSxeRM7VALZ59Y2s7efij?= =?us-ascii?Q?MZWc1OHZBtxsjd8JCcJCXA6o2G/RsEzFTLzbiHrATUTFe/3jQgdcw4NFmPHI?= =?us-ascii?Q?AkBJywW3gTKJ2F6Kg4QhWYFC+LSZOgQOQytvC0Uo89thTLflm2lquST8YDRR?= =?us-ascii?Q?AKkfRP4Bm7LU2iPEP4CW89FUMUgq1cKD7MbQHfdryU9hsK5Wo5joJn41GtwG?= =?us-ascii?Q?GanBNcb6L6Tv3Rc6fKS0/21BojowhDrlQbMBI6mJ?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB4929.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 0e38d89e-e543-4254-0025-08dd7f0801eb X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Apr 2025 06:04:12.4763 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: RPIv1VUyf6G1Xl9u10nW/H7NccsPP9HRrj8rdQbyGNuOjQGIpO4w4v4zWVseQ1Ao+iJhfp0MrRtHgfQrUflN66s854SiPw1fxFXQHAkt5nk= X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6258 X-OriginatorOrg: intel.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, 18 Apr 2025 23:04:46 -0700 Resent-From: michael.d.kinney@intel.com Reply-To: devel@edk2.groups.io,michael.d.kinney@intel.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ocXsJnieHnIRqPYLhUGDiIUhx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CO1PR11MB49290D6DA64F1D1E993871FDD2BE2CO1PR11MB4929namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=j38nOjpF; 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 --_000_CO1PR11MB49290D6DA64F1D1E993871FDD2BE2CO1PR11MB4929namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I would recommend not using V1 and updating callers to use V2 or V3. We could consider deprecating V1 since it is hard to use and has limitation= s. Mike From: Kun Qin Sent: Friday, April 18, 2025 3:46 PM To: Kinney, Michael D ; Ni, Ray Cc: devel@edk2.groups.io; Ni, Ray Subject: RE: A bug in the SmmCommunication V3 logic Current MM communicate DXE driver supports all 3 versions. I guess v1 is ne= ver invoked in Stmm based solutions. I will update the IPL code for SMM sho= rtly. Regards, Kun From: Kinney, Michael D > Sent: Friday, April 18, 2025 3:37 PM To: Kun Qin >; Ni, Ray = > Cc: devel@edk2.groups.io; Ni, Ray >; Kinney, Michael D > Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic Take a look at this branch with one commit that implements my proposal so f= ar https://github.com/mdkinney/edk2/commits/MdeModulePkg_Core_PiSmmCore_Virtua= l_Address_Fix/ What version of MM Communicate is used with STMM? Mike From: Kun Qin > Sent: Friday, April 18, 2025 3:34 PM To: Kinney, Michael D >; Ni, Ray > Cc: devel@edk2.groups.io; Ni, Ray > Subject: RE: A bug in the SmmCommunication V3 logic Okay, I can change that. On the other hand, how do we want handle the difference between MM communic= ate for stmm and traditional MM? stmm expects the incoming to be virtual an= d will always touch the buffer. But traditional mm lucked out by never touc= hing the incoming. Regards, Kun From: Kinney, Michael D > Sent: Friday, April 18, 2025 3:23 PM To: Kun Qin >; Ni, Ray = > Cc: devel@edk2.groups.io; Ni, Ray >; Kinney, Michael D > Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic I think my proposal is better to have separate logic. One for V1/V2 and an= other for V3. I agree that V1 must pass in non NULL CommSize and we can do spec change fo= r that since NULL CommSize is not implementable. V2 can support CommSize optional because it has phys and virt. V3 does not have CommSize. Mike From: Kun Qin > Sent: Friday, April 18, 2025 3:18 PM To: Kinney, Michael D >; Ni, Ray > Cc: devel@edk2.groups.io; Ni, Ray > Subject: RE: A bug in the SmmCommunication V3 logic Hi Mike, I created a PR on edk2 site but I was thinking about redoing it... Because = of the v1 implementation is a bit chaotic. The original form are still broken if we back out the change, according to = our discussion below. Specifically for v1, we cannot touch the incoming buf= fer after going virtual at all, because we need the incoming buffer to be p= hysical to populate the gSmmCorePrivate->CommunicationBuffer pointer. Other= wise, the validation inside SMM will work pass. On the StMm side, we are partially covered because MM communicate on standa= lone mm will always intake the incoming and copy it over to the static buff= er `CopyMem ((VOID *)(UINTN)mMmCommonBuffer.PhysicalStart, CommBuffer, Buff= erSize);`. However, this bears the assumption that we will need to pass in = virtual address when invoking Stmm backed MM communicate v1 to make it work= , which is a contradictory expectation compared to traditional MM communica= te protocol. One solution is to make comm size a requirement for v1, and v2/3 can check = the incoming content through the virtual address. Please let me know your t= houghts. Thanks, Kun From: Kinney, Michael D > Sent: Friday, April 18, 2025 2:34 PM To: Kun Qin >; Ni, Ray = > Cc: devel@edk2.groups.io; Ni, Ray >; Kinney, Michael D > Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic Hi Kun, I have a proposed solution that puts SmmCommunicationCommunicate() back to = its original form, so there are no changes in behavior for SmmCommunication= Communicate() or SmmCommunicationMmCommunicate2(). I then pull the V3 only logic into MmCommunicationMmCommunicate3(). It has some code duplication, but makes it easier to understand and uses th= e virtual and physical pointers in the right places for the V3 implementati= on. What were you thinking? Mike From: Kun Qin > Sent: Friday, April 18, 2025 11:26 AM To: Kinney, Michael D >; Ni, Ray > Cc: devel@edk2.groups.io; Ni, Ray > Subject: RE: A bug in the SmmCommunication V3 logic Hi Mike, Thanks for the explanation. That makes sense. I confirmed the issue on PiSm= mIpl side. A patch should be available some time today after I test both tr= aditional MM and Standalone MM paths. Regards, Kun From: Kinney, Michael D > Sent: Friday, April 18, 2025 8:59 AM To: Kun Qin >; Ni, Ray = > Cc: devel@edk2.groups.io; Ni, Ray >; Kinney, Michael D > Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic Hi Kun, I think the reason it may have always worked is for the case where SmmCommu= nicationCommunicate() at runtime after SetVirtualAddressMap() and CommSize = is not NULL. In that case, CommunicateHeader that points to CommBuffer is = never dereferenced and a call to mSmmControl2->Trigger() is made. If CommSize is NULL, then MessageLength field of CommunicateHeader would be= dereferenced as a physical address at OS runtime after SetVirtualAddressMa= p() and a page fault would occur. My guess is that CommSize is never NULL,= and that is why we have been lucky. With the addition of the check for the V3 GUID, the HeaderGuid field of Com= municateHeader is always dereferenced. So if CommunicateHeader is a physic= al address at OS Runtime after SetVirtualAddressMap(), then a page fault wi= ll always occur. No more luck. Mike From: Kun Qin > Sent: Friday, April 18, 2025 12:30 AM To: Ni, Ray > Cc: Kinney, Michael D >; devel@edk2.groups.io; Ni, Ray > Subject: RE: A bug in the SmmCommunication V3 logic Hi Ray, I will verify the code first thing tomorrow morning. But I just looked at t= he code flow before the change, it seems that SmmCommunicationMmCommunicate= 2 is also using physical address, and the common routine will deference the= pointer to read message length as well. I just checked variable runtime dr= iver did not convert the input physical pointer. Would that cause the same = issue? Did I miss something or we have lucked out all along? Regards, Kun From: Ni, Ray > Sent: Thursday, April 17, 2025 11:45 PM To: Kun Qin > Cc: Kinney, Michael D >; devel@edk2.groups.io; Ni, Ray > Subject: [EXTERNAL] A bug in the SmmCommunication V3 logic Hi Qin, I think there is a bug in the SmmCommunication protocol implementation. All 3 communication protocol calls go to the same communicate() function th= at tests the HeaderGuid against the V3 GUID. But when the call is from runtime, reading the HeaderGuid using the physica= l address of communication buffer would cause page fault. The virtual addre= ss should be used. The bug was not there without your patch because the communicate routines h= appened not to read any bytes from the communication buffer but simply pass= the address to SMM. SMM expects the physical address because the virtual-t= o-physical mapping in SMM is identical. The bug exists in both the SmmIpl.c in MdeModulePkg and the MmCommunication= Dxe.c in StandaloneMmPkg. The bug would cause OS boot failure if there is any communication protocol = invocation after ExitBootService. I guess the bug might not be there in your first version of patch, but was = introduced when I asked you to consolidate the logic together. Can you kindly reproduce it locally and send out a fix after confirming? Thanks, Ray -=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 (#121274): https://edk2.groups.io/g/devel/message/121274 Mute This Topic: https://groups.io/mt/112327494/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- --_000_CO1PR11MB49290D6DA64F1D1E993871FDD2BE2CO1PR11MB4929namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

I would recommend n= ot using V1 and updating callers to use V2 or V3.

 

We could consider d= eprecating V1 since it is hard to use and has limitations.

 

Mike

 

From: Kun Qin <Kun.Qin@microsoft.= com>
Sent: Friday, April 18, 2025 3:46 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray &l= t;ray.ni@intel.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
Subject: RE: A bug in the SmmCommunication V3 logic

 

Current MM communicate DXE driver supports all 3 ve= rsions. I guess v1 is never invoked in Stmm based solutions. I will update = the IPL code for SMM shortly.

 

Regards,

Kun

 

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 3:37 PM
To: Kun Qin <Kun.Qin@mic= rosoft.com>; Ni, Ray <ray.ni@= intel.com>
Cc: devel@edk2.groups.io= ; Ni, Ray <ray.ni@intel.com>;= Kinney, Michael D <michae= l.d.kinney@intel.com>
Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic<= /o:p>

 

Take a look at this= branch with one commit that implements my proposal so far

 

https://github.com/mdkinney/edk2/commits/MdeModulePkg_Core_PiSmmCor= e_Virtual_Address_Fix/

 

What version of MM = Communicate is used with STMM?

 

Mike

 

From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 3:34 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io= ; Ni, Ray <ray.ni@intel.com><= br> Subject: RE: A bug in the SmmCommunication V3 logic

 

Okay, I can change that.

 

On the other hand, how do we want handle the differ= ence between MM communicate for stmm and traditional MM? stmm expects the i= ncoming to be virtual and will always touch the buffer. But traditional mm lucked out by never touching the incoming.=

 

Regards,

Kun

 

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 3:23 PM
To: Kun Qin <Kun.Qin@mic= rosoft.com>; Ni, Ray <ray.ni@= intel.com>
Cc: devel@edk2.groups.io= ; Ni, Ray <ray.ni@intel.com>;= Kinney, Michael D <michae= l.d.kinney@intel.com>
Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic<= /o:p>

 

I think my proposal= is better to have separate logic.  One for V1/V2 and another for V3.<= o:p>

 

I agree that V1 mus= t pass in non NULL CommSize and we can do spec change for that since NULL C= ommSize is not implementable.

 

V2 can support Comm= Size optional because it has phys and virt. 

 

V3 does not have Co= mmSize.

 

Mike

 

 

 

From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 3:18 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.groups.io= ; Ni, Ray <ray.ni@intel.com><= br> Subject: RE: A bug in the SmmCommunication V3 logic

 

Hi Mike,

 

I created a PR on edk2 site but I was thinking abou= t redoing it… Because of the v1 implementation is a bit chaotic.=

 

The original form are still broken if we back out t= he change, according to our discussion below. Specifically for v1, we canno= t touch the incoming buffer after going virtual at all, because we need the incoming buffer to be physical to popul= ate the gSmmCorePrivate->CommunicationBuffer pointer. Otherwise, the val= idation inside SMM will work pass.

 

On the StMm side, we are partially covered because = MM communicate on standalone mm will always intake the incoming and copy it= over to the static buffer `CopyMem ((VOID *)(UINTN)mMmCommonBuffer.Physica= lStart, CommBuffer, BufferSize);`. However, this bears the assumption that we will= need to pass in virtual address when invoking Stmm backed MM communicate v1 to make = it work, which is a contradictory expectation compared to traditional MM co= mmunicate protocol.

 

One solution is to make comm size a requirement for= v1, and v2/3 can check the incoming content through the virtual address. P= lease let me know your thoughts.

 

Thanks,

Kun

 

From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 2:34 PM
To: Kun Qin <Kun.Qin@mic= rosoft.com>; Ni, Ray <ray.ni@= intel.com>
Cc: devel@edk2.groups.io= ; Ni, Ray <ray.ni@intel.com>;= Kinney, Michael D <michae= l.d.kinney@intel.com>
Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic<= /o:p>

 

Hi Kun,<= /span>

 

I have a proposed s= olution that puts SmmCommunicationCommunicate() back to its original form, = so there are no changes in behavior for SmmCommunicationCommunicate() or Sm= mCommunicationMmCommunicate2().

 

I then pull the V3 = only logic into MmCommunicationMmCommunicate3().

 

It has some code du= plication, but makes it easier to understand and uses the virtual and physi= cal pointers in the right places for the V3 implementation.

 

What were you think= ing?

 

Mike

 

 

From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 11:26 AM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.group= s.io; Ni, Ray <r= ay.ni@intel.com>
Subject: RE: A bug in the SmmCommunication V3 logic

 

Hi Mike,

 

Thanks for the explanation. That makes sense. I con= firmed the issue on PiSmmIpl side. A patch should be available some time to= day after I test both traditional MM and Standalone MM paths.

 

Regards,

Kun

 

From: Kinney, Michael D <<= a href=3D"mailto:michael.d.kinney@intel.com">michael.d.kinney@intel.com>
Sent: Friday, April 18, 2025 8:59 AM
To: Kun Qin <Kun= .Qin@microsoft.com>; Ni, Ray <ray.ni@intel.com>
Cc: devel@edk2.group= s.io; Ni, Ray <r= ay.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Subject: [EXTERNAL] RE: A bug in the SmmCommunication V3 logic<= /o:p>

 

Hi Kun,<= /span>

 

I think the reason = it may have always worked is for the case where SmmCommunicationCommunicate= () at runtime after SetVirtualAddressMap() and CommSize is not NULL.  = In that case, CommunicateHeader that points to CommBuffer is never dereferenced and a call to mSmmControl2->Trigger= () is made.

 

If CommSize is NULL= , then MessageLength field of CommunicateHeader would be dereferenced as a = physical address at OS runtime after SetVirtualAddressMap() and a page faul= t would occur.  My guess is that CommSize is never NULL, and that is why we have been lucky.

 

With the addition o= f the check for the V3 GUID, the HeaderGuid field of CommunicateHeader is a= lways dereferenced.  So if CommunicateHeader is a physical address at = OS Runtime after SetVirtualAddressMap(), then a page fault will always occur.  No more luck.=

 

Mike

 

From: Kun Qin <Kun.Qin@microsoft.com>
Sent: Friday, April 18, 2025 12:30 AM
To: Ni, Ray <ray.ni@i= ntel.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io= ; Ni, Ray <ray.ni@int= el.com>
Subject: RE: A bug in the SmmCommunication V3 logic

 

Hi Ray,

 

I will verify the code first thing tomorrow morning= . But I just looked at the code flow before the change, it seems that SmmCo= mmunicationMmCommunicate2 is also using physical address, and the common routine will deference the pointer to read message= length as well. I just checked variable runtime driver did not convert the= input physical pointer. Would that cause the same issue? Did I miss someth= ing or we have lucked out all along?

 

Regards,

Kun

 

From: Ni, Ray <ray.ni@intel.com>
Sent: Thursday, April 17, 2025 11:45 PM
To: Kun Qin <Kun= .Qin@microsoft.com>
Cc: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io= ; Ni, Ray <ray.ni@int= el.com>
Subject: [EXTERNAL] A bug in the SmmCommunication V3 logic

 

Hi Qin,=

I think= there is a bug in the SmmCommunication protocol implementation.=

&n= bsp;

All 3 c= ommunication protocol calls go to the same communicate() function that test= s the HeaderGuid against the V3 GUID.

But whe= n the call is from runtime, reading the HeaderGuid using the physical addre= ss of communication buffer would cause page fault. The virtual address shou= ld be used.

The bug= was not there without your patch because the communicate routines happened= not to read any bytes from the communication buffer but simply pass the ad= dress to SMM. SMM expects the physical address because the virtual-to-physical mapping in SMM is identical.<= /o:p>

&n= bsp;

The bug= exists in both the SmmIpl.c in MdeModulePkg and the MmCommunicationDxe.c i= n StandaloneMmPkg.

The bug= would cause OS boot failure if there is any communication protocol invocat= ion after ExitBootService.

&n= bsp;

I guess= the bug might not be there in your first version of patch, but was introdu= ced when I asked you to consolidate the logic together.

&n= bsp;

Can you= kindly reproduce it locally and send out a fix after confirming?

&n= bsp;

Thanks,=

Ray

_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#121274) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_CO1PR11MB49290D6DA64F1D1E993871FDD2BE2CO1PR11MB4929namp_--