From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 9E50E941B5B for ; Thu, 29 Feb 2024 19:41:15 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=JxFUeU4nse5pb2sozYY2ef9iZ4V2eHLRQE/qG0O3kBE=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:Autocrypt:In-Reply-To:Feedback-ID:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1709235674; v=1; b=ssn9C7Qe7i5GwCon3cCnzLVqNUXRnM09Q1HHpAww4SiqdAu6Gsu0f2/bL6r0b+4EGysyEMYH zvs0T7S0sbVrnZtsXAPQO/9HcLsvO03Y4zxO3F286X0fpz4zNv1bjzjzx2+NrnRRiC7yRcwv7d4 RaYtJNThGLAwN5WMluBuIwnk= X-Received: by 127.0.0.2 with SMTP id QqviYY7687511xcPCd63Mufx; Thu, 29 Feb 2024 11:41:14 -0800 X-Received: from a7-20.smtp-out.eu-west-1.amazonses.com (a7-20.smtp-out.eu-west-1.amazonses.com [54.240.7.20]) by mx.groups.io with SMTP id smtpd.web11.4580.1709235672981177594 for ; Thu, 29 Feb 2024 11:41:13 -0800 Message-ID: <0102018df6628fb4-14e85f28-9225-4d90-a6fd-f6c33530d484-000000@eu-west-1.amazonses.com> Date: Thu, 29 Feb 2024 19:41:10 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH 2/2] MdeModulePkg/DxeCore: Fix stack overflow issue due to nested interrupts To: devel@edk2.groups.io, michael.d.kinney@intel.com, "Ni, Ray" Cc: Liming Gao , Laszlo Ersek , Paolo Bonzini References: <20240229130246.3-1-ray.ni@intel.com> <20240229130246.3-3-ray.ni@intel.com> <0102018df508a238-cb6f0ada-c808-4b77-8555-2315d3377c96-000000@eu-west-1.amazonses.com> <0102018df5f307dd-f8dad33b-e6d3-43dc-baa8-2ced62ecc21e-000000@eu-west-1.amazonses.com> From: "Michael Brown" Autocrypt: addr=mcb30@ipxe.org; keydata= xsFNBGPmfF4BEAC3vcU4aLC/9Uy/rTpmYujbqxQNZje9E34jGvLxO3uYwj4BeHj1Nn5T2TDM Gkc4ngk+mGPsJsIn69YU5cfVN+ch9O7FVfsn6egZsCNeLy6Qz0o//gBaWJodFBeawuBjXXyV HnQZa1p7bA/Lws8minW7NrZ7XZgEBaiVm1v1dNbLEoWR8UL2AMtph5loCQ5jPYQNqp/wH9El /R30GjXvAd1riWyJR2TWSN23J9rnuH2Ue+N4yEnWxAsBQ6M/NFQ5z42w4mYdsnzy1w3PulrL icpSixXHkm3lQcKGtKKX41HvJukSpxCgbHfuHGEJZ7bdhgRic1DHKav0JR8kQhx3gnPh06z8 1Teu2NKkSsTR3Iv6E2x6Yy6H34lKWzBzd8TLNSevesDD/L6NU/HxT9AxrTBuypk9PZGe2VH1 W03XnR/0Mnr0QqQBXcIAERdgNzRJY4VKF75vedf8IooZFUQ4RUlqH+x3aZB9nJ9ET77mPaNi SQVQBxE68uzb7eh2Kf6z7ftOYpWPw1v5HyB3oMmafEDG36SIvNF2wnmNaLQDRnAbTcy4ERgy tpJ3wtQDJeXOePLv8hJ3q7DSuePl7cwz4xy0ZHglW/EXRXLnyRRACfDGowyENoStg06qF+qm edGu1wNtmDZ/lypWm/CkzzpUDFeGP5BLZlqwVX4hn88llfvVzwARAQABzR5NaWNoYWVsIEJy b3duIDxtY2IzMEBpcHhlLm9yZz7CwZEEEwEIADsWIQTgD69MBpjBm2slMvwCNbEKAOtEUAUC Y+Z8nwIbAwULCQgHAgIiAgYVCgkICwIEFgIDAQIeBwIXgAAKCRACNbEKAOtEUFlhD/9ElIUg JxBXpIbF8s7u79OdXLld2Z1DfVmhP5Q+GilPvEeAWHhp689S9B88aNvpwW5zJfxlxcJZO0ay jc7E/vtdNrkXGWNEEXBgdve6m+uL+pW/i5E2htqxbLyfgTJKmsvJ8graHbwrrBS/PA8KuwVJ eAGbBNi3f1gyQQWrLqfTkUpLtuj7A76iVVk0G0a78L69Al84qhK2imqpFJoZt1F8h0Z5ddGv mvf2M/DZp87UXvXjy7X6r7msbMZa6S/Jv0dtWHeZGl3Xu3qzbtjlqFyz2Q7TibHiirsgg/CV BsbH/LLbi/aNCCQ/85C6jAMB0lNzcVZ7ZiKKo+vBNMTycDFk70LA9yjlNf7exHejoXmPkLmH ddapYZ4dzwdOiJlaTu8NZgzXUCt3RDDA1qmZrAOBF/F+tPILAEhenl9kj3blD3mPV2SrWLWY dbahY9BsylUhj/qE1ik5CJXrPotmJhok9Vpg07xKDpVnZXuWLGNIE8018UumO7phLrWQwLb1 wJdN7PG165w4UWf4aQphfwaMKOVU3WDghz3aVSP9rgtm3RsUcYHPKx8IaPcDh2yf0bgG386i Axx3U3UQeyz2Pb9Vigo6DmPwXjLkFr/dukvVLVJLVkUab9ZhhERzWTEEMifUVEK2rGNvA87L VKJ2zOyxWx1e0CPj6fcGbkJ0D10XLs7BTQRj5nxeARAAz18zv2ksRiM6eEKG0qzpiKHVYlVy wtjla+m9wuAIwm314tffY5hjQN46uwTstdhQirjywF1EmcS6KNGiIjmoLim+dqyFP5d/UF5A VjLt0TYq7HjadIxbm2/CvcRnNJ01FkD99xLxV0hFTUAWAUX1mNqQ3MmWIjV89wiT06uuAUog m+jG3RRDyWbUnVELR60mhzccKsaEsjO/HqIERvBwL7tlOJewlPrVyz9Zed9Nhhv0KDAYmdEm kIEEbOfsjRu5I6nIY3NrX+QP9+nmgxADlsjvLXTSU0fT/g7IPEl3gpsQZAbgmrlGcPtvXod8 P4iOmL8GJDU1RdBE9TBOLEbu9UlDRD4zr6tdzRpB9wvXdtSUcNCdHVqJTfq2qjIlBk7x+zQD ayhxzDvTMxD/93K6txKXmVVtfMBsmt9KuD2JBUEAExjsLHqzg48nQg8wF9JYWCWGBb36qpd0 yC6VPzhSLe2Ov3/GyV5ZshO046+OiGxEeaHCwMnDTZF9xrQ5paCwWedlWKvGM2zB64AHuk+M v2ABK/gbDO7eS6p+xz11oD1NHr1HQLRtknfClIqj9AmjgX9maD+4GUrmHaxmkNilIukahotd Un9Up2gX05Wy/S3H/v8RB0kxwWg2Wh065dnyCF4Doe18bcYZvM+iMJmUBag6aDfQlryM04K7 z4ITYDkAEQEAAcLBdgQYAQgAIBYhBOAPr0wGmMGbayUy/AI1sQoA60RQBQJj5nxeAhsMAAoJ EAI1sQoA60RQZj4QAIkiRDVNWynZ4kEdpqmf6hpD++Zycz+LMne4iGRsiyyTf/rPNgskNLrU JD555yDvFiEAhOI27R8YNCJj5byXRDa/Bm6ueClFia+POibt28UEdyOFU9PVcgFaU+VxaBIP rHacHL6A7UKFjmBN7o8VkVF2xXlmFge795mP4/Y3t6qfWUTodrpw1w1t5/bZxZdWqX4pUCpY fEx87jm60+Mj0Tb4VPWXz0UD1q1BDcdYxNa2ISLaJhGJmjjks9eqdFOhPo1fTINMNWF2Alxi jA6WNT8nn9lm1kav75EMYMc8WIR9tb03i+IuKNp2IWwTGBqIUyQj00BhHkZQFl4HxZhV0gXE AWu34Q/Z7hOUXGXq2tvYCxDeaQb2wks93e62lrrUm1JGhPWkVoCI8Md8N2mkonqIfMK8lQ0W WbkYHdKBkgDqhDypNNhkjWNX3JL1kL0c3rqGL381iBAZaGQPygyCx2xH9PDNp59W6u8sXb13 +UX+kXdWU+KYbMTVoO/t4MxUJg6nXPJHz9NCkyluI820l+2OtXZZy0u196evIlUdD6RoTrNK z5OgFxNctVi9BPsQea9du+JlYJ460vZNPz180oczj7iqffd+p9DmAkeK25njWhg3qPeXiNZN 45J9eMChSOaJ0GMGUQndIIxz7PO8IzjbkSHLG5CKrR3MaphMB/0L In-Reply-To: X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Feedback-ID: 1.eu-west-1.fspj4M/5bzJ9NLRzJP0PaxRwxrpZqiDQJ1IF94CF2TA=:AmazonSES X-SES-Outgoing: 2024.02.29-54.240.7.20 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 Reply-To: devel@edk2.groups.io,mcb30@ipxe.org List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: sFzF7WyEBIUKxJcbUo17Jd5yx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=ssn9C7Qe; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 29/02/2024 19:09, Michael D Kinney wrote: >> "When the DXE Foundation is notified that the EFI_CPU_ARCH_PROTOCOL has >> been installed, then the full version of the Boot Service RestoreTPL() >> can be made available. When an attempt is made to restore the TPL >> level >> to level below EFI_TPL_HIGH_LEVEL, then the DXE Foundation should use >> the services of the EFI_CPU_ARCH_PROTOCOL to enable interrupts." >=20 > I would claim that this spec is perhaps incomplete in this area that > that incomplete description is what allows the window for interrupt > nesting to occur. This language is correct for UEFI code that calls > Raise/Restore TPL once the CPU Arch Protocol is available. It does > not cover the required behavior to prevent nesting when processing > a timer interrupt. This could be considered a gap in the UEFI/PI > spec content. I think it's important that we don't phrase it as preventing interrupt=20 nesting. The UEFI design *requires* that nested interrupts be allowed=20 to happen, since callbacks at TPL_CALLBACK are allowed to wait for=20 events at TPL_NOTIFY, and this can't happen without the existence of=20 nested interrupts. The problem is not nested interrupts per se: the problem is the=20 potential for unlimited stack consumption. >> - How does the proposed patch react to an interrupt occurring >> (illegally) at TPL_HIGH_LEVEL (as happens with some versions of >> Windows)? As far as I can tell, it will result in mInterruptedTplMask >> having bit 31 set but never cleared. What impact will this have? >=20 > This behavior could potentially break any UEFI code that sets TPL to > TPL_HIGH_LEVEL as a lock, which can then cause any number of > undefined behaviors. I am curious of you have a way to reproduce > this failure for testing purposed. >=20 > I would agree that any proposed change needs to comprehend this > Scenario if it can be reproduced with shipping OS images. https://bugzilla.redhat.com/show_bug.cgi?id=3D2189136 was the original bug= =20 report in which it was discovered that Windows 11 would call=20 RaiseTPL(TPL_HIGH_LEVEL) and then enable interrupts using the STI=20 instruction. It would be interesting to hear from anyone at Microsoft as to why this=20 happens! >> - How does the proposed patch react to potentially mismatched >> RaisedTPL()/RestoreTPL() calls (e.g. oldTpl =3D RaiseTPL(TPL_CALLBACK) >> followed by RaiseTPL(TPL_NOTIFY) followed by a single >> RestoreTPL(oldTpl))? >=20 > The proposed patch only changes behavior when processing a timer > interrupt. I do not think there would be any changes in behavior > for UEFI code that makes that sequence of calls. The patch affects all callers of RaiseTPL() and RestoreTPL(). Given=20 that it creates a new piece of shared state (mInterruptedTplMask), I'd=20 like to see some kind of proof that it can correctly handle an arbitrary=20 sequence of calls from unknown third-party code. For example: consider an interrupt at TPL_APPLICATION with a third-party=20 timer interrupt handler that does something like: OldTpl =3D RaiseTPL (TPL_HIGH_LEVEL); ... send EOI, call timer tick function, etc ... if (OldTpl < TPL_NOTIFY) { RestoreTPL (TPL_NOTIFY); ... do some weird OEM-specific thing ... } RestoreTPL ( OldTpl ); This is arguably a valid sequence of calls to RaiseTPL()/RestoreTPL().=20 With the patch as-is, mInterruptedTplMask will have flagged the=20 TPL_APPLICATION bit but not the TPL_NOTIFY bit, and so the call to=20 RestoreTPL(TPL_NOTIFY) *will* re-enable interrupts, which is against the=20 intention of the patch. Thanks, Michael -=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 (#116187): https://edk2.groups.io/g/devel/message/116187 Mute This Topic: https://groups.io/mt/104642317/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-