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 315DD740032 for ; Sat, 19 Apr 2025 03:32:15 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=2ZlscWNhuus1EENYYwpzmR9a3TYeQhE8oMXWnLUMVf4=; 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=1745033535; v=1; x=1745292734; b=rOXu8p2eaO/gZVNGMWrbgkQjOPJyyqygdOdr6AXs+GgmrmBywDN4nAFHXamRYaRN5H7ZjPgt 7vn5TY7AIFPrOPPWeHcmEW739lIEjogVHhSuAS6UaoytbjUqWN2CsFRcaoM5rZrmksjtzy4PMjZ oE7/iZUXwIFTGRXWK+pKpPo73ZkbUnq7F7qdOFJK6MGltpV4X/1PkPeowdMXVskmXNgBSFz345k k83lR6AIHzrbuzs8rBqThr3UC/gQlyGHsyfLMWJMVSim74pB8NTcbty3jloWgwvS/xXh4iwqDnv 7QAAnJnC7moYlmtuws9Ifb/b0CdRcFDUX4moZs2ZblLnA== X-Received: by 127.0.0.2 with SMTP id 5SkfYY7687511x08zPrC4DHJ; Fri, 18 Apr 2025 20:32:14 -0700 X-Received: from BN1PR04CU002.outbound.protection.outlook.com (BN1PR04CU002.outbound.protection.outlook.com [52.101.56.127]) by mx.groups.io with SMTP id smtpd.web11.999.1745015655295767287 for ; Fri, 18 Apr 2025 15:34:15 -0700 X-Received: from CH3PR21MB4517.namprd21.prod.outlook.com (2603:10b6:610:21b::17) by CH2PR21MB1384.namprd21.prod.outlook.com (2603:10b6:610:8c::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8678.12; Fri, 18 Apr 2025 22:34:12 +0000 X-Received: from CH3PR21MB4517.namprd21.prod.outlook.com ([fe80::7bdd:bb65:8eb1:f082]) by CH3PR21MB4517.namprd21.prod.outlook.com ([fe80::7bdd:bb65:8eb1:f082%4]) with mapi id 15.20.8678.011; Fri, 18 Apr 2025 22:34:12 +0000 From: "Kun Qin via groups.io" To: "Kinney, Michael D" , "Ni, Ray" CC: "devel@edk2.groups.io" , "Ni, Ray" Subject: Re: [edk2-devel] A bug in the SmmCommunication V3 logic Thread-Topic: A bug in the SmmCommunication V3 logic Thread-Index: AQHbsCxo5nQBH4RMRk+1qEepe02xqLOpBL4QgACO8lCAAComQIAAMy/AgAAC4LCAAAuzgIAAAzIg Date: Fri, 18 Apr 2025 22:34: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: CH3PR21MB4517:EE_|CH2PR21MB1384:EE_ x-ms-office365-filtering-correlation-id: adc71b84-0401-4a8d-c01c-08dd7ec92478 x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam-message-info: =?us-ascii?Q?SfQ6ODohno37LmNFTRIkAXoJJuJ4Zw3siLfDm56dNNPL+Ng9u4qrs0WsrBIM?= =?us-ascii?Q?IexViXyMTb8KQM1dxV/Fv14URDi1yek+dwFl/XrmuWDLGDqxucjkM0yqIMOt?= =?us-ascii?Q?QmuymhL50nkTXQB9bodkUMdNGvf4FZnxb2vFdQChqmaTD68a8iC9iXjLgBMj?= =?us-ascii?Q?WVaKEDlAF0b2531ZBKInWC7WRaeSkD660pTQpEfKSONabYXbzGcF4BTtHndh?= =?us-ascii?Q?bWbGYIZz4i2ItePULwDfPD9DSFHx7KaKA8gxCoPab3/oc8jfMvMqItsji/dX?= =?us-ascii?Q?RuuF9jfRYKKeYomjYPOmW2ED2MjV/35i9cjHC4cyEOMLnOTQ1l7A+xfuCwXp?= =?us-ascii?Q?X47PZSozFft8v8mqyq6MZ218DHWloYtM6Om1SvCOt4UVUMwVo12pUEabFeFE?= =?us-ascii?Q?cWstfDaBwWLgmn/pgqLHLmi4e9pwoTDvcv2Fet+IOQQ+t7VPeRYRrKk0uqqV?= =?us-ascii?Q?dhlMCXx4LqTuNYYB/We07Iw/5Xgo0jVdgbe0kjsW/n+VNAj5ZWEC2akDrL/p?= =?us-ascii?Q?K5opZSVTWQlw1h9XsbEgwWGBz1a4J+oAUJAlJCeYyMH+E7hDuF4YyGAy8JfF?= =?us-ascii?Q?Jqb9QY1ksR7Nqob0D7+DdbT964rnhXB2ll7fRyFGYspDryQw8lrfB2zJ1RQk?= =?us-ascii?Q?wMK8cxGj5j9u2LVAgsIwbLavORv3qSSGHW91dNyUD3BHuWhdusJN06nMdfUB?= =?us-ascii?Q?wbbXgYgphLYyey3WLM/Gc8VkbGfMQyv6r1MEbl1JQZNSQfX3JvgB3f6QU5oH?= =?us-ascii?Q?7uW8UhwAF+A2J/59CkYM3c1Yf3evm57pd9LUcE40OY3KGv/X4zrHCtCFaZCR?= =?us-ascii?Q?MuIHFMAOWidjlgXZDYA/Pvj3u57yKsuBAG41KXyTVzAUD2bfQMZ3/7mNXVmC?= =?us-ascii?Q?rEQjoQcKa1pPGN/aqFncfcu7So3TAnI5hrhoO735HZHCt111Ts2D62STcuZ4?= =?us-ascii?Q?Zn2wacfRXHdahd0kyb126uVsqjDmp4RgwvVh078X1xQ/hnXgZILFigyYy6xW?= =?us-ascii?Q?/IPeVwcpBfmzOnl1U9CvYctf2KnyPFU6Vzx+EATPCh3tFzyL/+HWrxyXRXCe?= =?us-ascii?Q?Z6wviIGxzSlWsC4QFqlI/cT1WkAQ8pRtKF6Ld1WJExrcwqL5kb4GLbVWUdZG?= =?us-ascii?Q?TXwvnredf7qzKm3RTfvM4kKc29YH4+5827SjeUGC+XYV+b3Y2B0ohhfZpG5A?= =?us-ascii?Q?NZlHLidgkkOes+5goDjRN41RiqmYrVm23AJ8vOHArpFsgQMwAS9OLb8TB0up?= =?us-ascii?Q?5WR0zYbBgLAMUdt8GkJCqBJk9JLSp60kHadS9/BxdNQPPu6gfL7SQ7AXodYY?= =?us-ascii?Q?a3EmblQa2c0kJuErZXSRhDCTD3FJkJ5kizE2q23GE2luJIy448fWpDUpEH/w?= =?us-ascii?Q?OHCFzsCFumvgpYq621eUarkydVUcRP9WIr4CBmbwWNGCX51GuRjGlSV5aMwX?= =?us-ascii?Q?kAbB38y18DbOicbw5zdCqTACeTKbetnwgxcXyH8pKHU/4eVXa+wV1g=3D=3D?= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?/BPhgTP/Igm9Qzyj+dOIzsu7xWdjEmDjThBJAa5oVOxtDk7vXZEVKc6gIuXv?= =?us-ascii?Q?Bres+uA+Pu5dQEQnuCAWegpGfYSW85sJBoRH0xGBZoeeh4L5/cXxPlS0DlqH?= =?us-ascii?Q?57jPsHds8SciAhKdgbrWI6dIxnbQIVl86VxdlEu3Jf9OCJAybbwPZlr3JAMt?= =?us-ascii?Q?zIuvCs1QR75RjOtV8wzgXiC+glYeu//1Z8TTIuCNwaoRitV2CjZQCyqulIvC?= =?us-ascii?Q?JWhbs0ziMRw4JWc47u8mWfU1P50PLW+gDvcF0AVuz+tDkqur0mbw4LQ9pzX2?= =?us-ascii?Q?QIcELnC1vjFds1CVXSeN1Sc4OZD/kP26MffdULq7Q82EaW19uPTIyoLND+sq?= =?us-ascii?Q?ase2EvePdqzXj/UDpdV6m17WWAn+mIK1kk6TgkTU97TGk1Mha385kTSbbL+m?= =?us-ascii?Q?T9p4rR5hdl2ofhLer8At2VJ1eoCtI4FrG8YzvGoGoqnYRBDm8OFtefPhX8//?= =?us-ascii?Q?+GliaoUbCnByebP8U9mJbysaPaShodTwClU5nsHeC+tGdRMQhOgwyicuoiEY?= =?us-ascii?Q?OfDHC66s+NnsAK54onGVFyhGK/BRrZUsMoiNxLxHBxCrS3jLyl5y+bfsLsVD?= =?us-ascii?Q?jQ1StACr/lLMGSC/s35gPOfKLOViO7J+mWJFAfk18uFCJhjRsuL1hwfz45OP?= =?us-ascii?Q?QXz0kctaMcmAFPEKF8bZI/VLlQ+p5hJW0EnSbfaEHHpGrUjWSRvKFSPpMiAF?= =?us-ascii?Q?vFRk8DZEIuTS+ZnnuzllLJPIEb4Yp81821DCkQbYMWiyCxQDSE5IM5EmapWs?= =?us-ascii?Q?PzlGrC34H3GerLA0BgzQOh0uDXSeZE4NkTpc76kz8LdJZVWLkJKMqmyqKZqP?= =?us-ascii?Q?sZkLjznC4+Q7dqcAH/dufyBHrUbIzFEtGE11D5Afgl11gAP4s9QS3cq/zgNS?= =?us-ascii?Q?sujXDUhucKirJ27ZKKFR7g8np/ykYWUG7YyV9deF6DaCyYkh3HQAb6f1udke?= =?us-ascii?Q?C6PzTEjg99UHhhZBwofZfezn/YnqPziCbd/wMQ85vHU5yW1xjxU93ilj1m0W?= =?us-ascii?Q?XJ4z1WHb25P343OvlCXsoKXbD0Sj4oZ+SXzFqouO1otapRwk+UJ+KDv/9wpN?= =?us-ascii?Q?f+YDETSuoOiXi01tkK1KVEfCjlIsI9ac6PoqAVdf0nesv+H+WAz50J4cz77r?= =?us-ascii?Q?CluEdZEFJY9sAgOfKXy1YpysFZQYd/jxAWdCbR6o3KL+uo+JalaRhkpfFyGg?= =?us-ascii?Q?7Uy9Q1bPWCTl7YiTgcor7cC3njDJWWQbwxFpq9sZA2kTUOHnsgK8o7EU69nV?= =?us-ascii?Q?5KtuXejlCiJXr94/sPMk6NRoaKHSWiza2fbId7G+NtqSn0Zy2nu23HcP6ZTd?= =?us-ascii?Q?MxVck+p2unh8Aret1HsCIXLrmDQCFVwE3q8/WGsk47/92X2WQAl2WEq9GPIR?= =?us-ascii?Q?hu4ylsN9S47b4BU7WvF0N42sK/V3AdDf+oUmHRHfGNxCI1ls6YsseNzEDpay?= =?us-ascii?Q?yAZ2bYzNZoc5Aps+EpZgRdPjgLzHlXJla8rz6Rv2l9rI4jQaLIV4UW6Vw3Rp?= =?us-ascii?Q?T3s9r0obuPHTzz/Xn5HMUXFhxPemElXf3gMVHygZc4y//tNTb/rcArVBxWLF?= =?us-ascii?Q?RKOxfSOR+MrZiUyy4RgZz3YtJnXnEXYK0w2rFnt/jUNcwXFGwyOirY3VnM4V?= =?us-ascii?Q?NixLGcG95fVXEOWqXC8ApkO99jdDgvI266oP9tqZr+T5?= MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CH3PR21MB4517.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: adc71b84-0401-4a8d-c01c-08dd7ec92478 X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Apr 2025 22:34:12.1597 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: 7gGrYWu6ujo3izdj3anS0gzm1LMehgE+AjSXxp/YdBP/qFj4Q5tGfV0OuilzFh1M3FGV4V3ryW+QHpONPEQZCA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR21MB1384 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 20:32:11 -0700 Resent-From: Kun.Qin@microsoft.com Reply-To: devel@edk2.groups.io,Kun.Qin@microsoft.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: fv9Kq4fHy4rikJHGvvsuCjFUx7686176AA= Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CH3PR21MB4517A5A8939B84087ACB2189E9BF2CH3PR21MB4517namp_" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240830 header.b=rOXu8p2e; 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_CH3PR21MB4517A5A8939B84087ACB2189E9BF2CH3PR21MB4517namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 (#121272): https://edk2.groups.io/g/devel/message/121272 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_CH3PR21MB4517A5A8939B84087ACB2189E9BF2CH3PR21MB4517namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

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@microsoft.com>; Ni, Ray <ray.ni@intel.= com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Kinney, = Michael D <michael.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 (#121272) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--_000_CH3PR21MB4517A5A8939B84087ACB2189E9BF2CH3PR21MB4517namp_--