From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (NAM11-DM6-obe.outbound.protection.outlook.com [40.107.223.93]) by mx.groups.io with SMTP id smtpd.web12.181.1606857132454778046 for ; Tue, 01 Dec 2020 13:12:12 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=CKZnpJwc; spf=pass (domain: microsoft.com, ip: 40.107.223.93, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jgUC+9kPBZHFBxYXL5i9GmfsuCCOtcqPyOUGpz+Nrr0rrpIzXxqc/UteF1xx9x8cRB6eMRdSpV8Wg0rFUVpJdW7ZDrJwReX1fwyiNNIG+egZETgHZSTjDo2Za0C1IHYXJe/Q+jcRF84ySQTngIIJoovtPAdyu0Tm8K7qY+fhnh8EkZWsXMue0ek8b9kFheKiobs2sRMNPIibMDHtDk/TEW8XYQeynGCqTC7RlX06qu3P5g4y+cMWIht2mOYGMlfKWN5eDx02A2ESD+3n7JfjqNr/5RrlihcUH7kVo0BNnYkyLOR7RaVpemkW5rX217bOu816eN9+lWO93DB7ZfUV1A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tBcYqZ/ocXyCBd9jWO3yN4mTdGK8/ewboXfKtZqigoQ=; b=U7lGI/HFig3WUhPUNCYGnlBJZ7BSIMw+qVz+oF00l0xQB2YYW+DRdDKwPFZOohNl78d1UhytxKr2SCEpHdPWAX0VOQQP5HEi+PshnM+6shnivSzxSuDP3QYgYkmrzoqN74uPRZBPscuRa3hMCSVGWPTIzr3vJLGsV39cTdLLnx/hwnhtyantPqP/hthbCLW1nkREDSmoMhRU8flBxKsjG31RQeuEaZs868OMLLl56PPznKMIYTkWQKRr2KK+eNeaAXRygSX7KER8V3gnUYL7x3ebtu2qMEP+DstQ0PkjieyLfR+2B4r87k8Zy7I73ag38HHSGfLFd6ALI9F5ij1nmg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=tBcYqZ/ocXyCBd9jWO3yN4mTdGK8/ewboXfKtZqigoQ=; b=CKZnpJwcEeQFQofOTncWG7PrXADw9qmI3iXjBjheDjgvFtl/0elNRXK9Aj8Z0M6Be43VRGKEHVLp8gxt2/7cf775EjHbyI4V1/N0rwWQuVHK4ICZM6plx/ATPtUHOh3nEgVQpQ/yqOcalNDj6f2AeiU5Mgeh6cXprxOOGAY0O7E= Received: from (2603:10b6:302:10::22) by MWHPR21MB0829.namprd21.prod.outlook.com (2603:10b6:300:76::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3654.2; Tue, 1 Dec 2020 21:12:10 +0000 Received: from MW2PR2101MB0890.namprd21.prod.outlook.com ([fe80::a807:8b5b:a667:d7c2]) by MW2PR2101MB0890.namprd21.prod.outlook.com ([fe80::a807:8b5b:a667:d7c2%4]) with mapi id 15.20.3654.002; Tue, 1 Dec 2020 21:12:10 +0000 From: "Bret Barkelew" To: "devel@edk2.groups.io" , "lersek@redhat.com" , Ard Biesheuvel , "jejb@linux.ibm.com" CC: "Liming Gao (Byosoft address)" Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable() Thread-Topic: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable() Thread-Index: AQHWw3BkciYYeYRn9EGdkWHG751GaqnixjWz Date: Tue, 1 Dec 2020 21:12:10 +0000 Message-ID: References: <414b7574bf8249de0cecd16fb422c711feb76e1a.camel@linux.ibm.com> <1b9adc6f-37e3-0a9b-29cc-2c97e8a9e0f5@arm.com>,<0f6e92d4-f600-8495-53af-5898a2cdbe2a@redhat.com> In-Reply-To: <0f6e92d4-f600-8495-53af-5898a2cdbe2a@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2020-12-01T21:11:32.2753726Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; dmarc=none action=none header.from=microsoft.com; x-originating-ip: [71.212.128.184] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 4723d2e2-d22b-4d31-eee2-08d8963dc43e x-ms-traffictypediagnostic: MWHPR21MB0829: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:6790; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: aOD4cFlrZIfqwnV7Z0suAC61zL9kAPTLi0OU3SxBp/3/EAUUqND02XQKdgJ/avTv2nyucY+IgESL9hO81z8BK+zNi34wznlbAXrSUo/IbzwSSAddc8k0W+blpH6Q7Lv4xS3OwMwm4uQLHGL95ufGqHstYhx93vyK+D9Qb3U0+Nh68pvwVBunMjJA6skBjjP7WJGOXXeINs2/W9kgvKBbW40HsaEIn0uX+X4GNuwwlDQge3/3RWnowFU/eb6PRkifxGGlKJn/hJ/0a23ZR5ykW9hr4hsPOlD0HZbe51ZhCpGy2XDNcLcrsHvsGaqStwPIr9cGQvse2k+gWjv3N9lN4puTq7m5PGAQZ63efwQQ34lxhdPF0TsYkrSHOzsI5QjFGgOZwPC2zxSGbrk7+javGA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:MW2PR2101MB0890.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(39860400002)(136003)(396003)(376002)(346002)(316002)(15650500001)(6506007)(53546011)(8676002)(82950400001)(7696005)(86362001)(82960400001)(110136005)(9686003)(2906002)(71200400001)(66946007)(478600001)(52536014)(966005)(10290500003)(5660300002)(166002)(76116006)(66446008)(33656002)(66476007)(55016002)(64756008)(66556008)(186003)(8990500004)(8936002)(26005)(4326008)(83380400001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: =?us-ascii?Q?coL0PAZ+ch9G2xsP/Cap2VZnv7zGcUbDlb418O4VD/q6n5UYzziTyjJsRaH2?= =?us-ascii?Q?cwxisRLVYM9lNFy5EwnU7YbmovQ1NdEWx0aK2QXRO3KCX/xv4YKgPFcSD9DW?= =?us-ascii?Q?GFmBgf9wB8oMTewChcJXuQoyhHHT8RAq5uUzhTNkaIaFnAmSUVRL6a0dkJN0?= =?us-ascii?Q?uYVfDPOYUlFcQFJd13djt6JJaSRIev76KhqlQPkvcmn6BmqUNZ2LE9BYZYV9?= =?us-ascii?Q?srZ1P6xr6dkLFuI84QeKw/gmktqajWVJY5Vyig3OzZAY4OBmFxGWyAoeDBTQ?= =?us-ascii?Q?iPKVtrNGPTHNox6hYWZ3ULu37fB3fWDPJP4xdhh/+D4sXP1P7uzB3UkcDdcl?= =?us-ascii?Q?epsOWy0MUiHh39uxz7Qr+WBlWl1/aHRmg99x6g6FrAPDVZ4aiC+wqeYrslps?= =?us-ascii?Q?O7uB9p1xiW7SYpngREMgp2p/xc72tvEOH9n7bCSoIaUs9xDJqWndKEgUfaU4?= =?us-ascii?Q?v5tx08ab9OmVDtNwJIiH7L4Dg7lidqvc0n+UN3Zr9oh0XqW4i4kjyqSpJiEk?= =?us-ascii?Q?xgk2fBE5r9S6dBvCq8/KZlCpJZQJ3ovGsy0k/O1RQqSB1qpG3/faEw60/LNu?= =?us-ascii?Q?zm1hHvRHROYihike3TfNIJ027+x5iBIVpB0pGaHg2Zim4qaW8QG2PXQJTSLl?= =?us-ascii?Q?2C4sTvRzNuw8tjpsarDahpqpGgTChPCDr/+InFfLh37k8V5Hb1ahxdXC7klq?= =?us-ascii?Q?kbSCYr6ugiM+KrZdzrxeqEBUZAtq3BIJjutZZ662Nqz0RFuBPdMbRxX4DcBI?= =?us-ascii?Q?FM20VdtJeeqpSJwRkv1R6tdTMkzKaZoCZw88xKqbYOyBsxWwtVhLsHNmbjvl?= =?us-ascii?Q?+OTGx4PHNeQ0mGOm4oV841zgG8lPEGeKjCvBb36GcFm3WHIs9E1sSXT4Y+Mg?= =?us-ascii?Q?WsVQ0xDkL0Sv/dGB//4V0NLqB07RjmMZmqL3tZdFhUv4+mra+LXB1e1XXEqj?= =?us-ascii?Q?q3aogEI9rDwi/y4NKV0RuNNB41rA5td2ki1K97wjKbpIH9/PC2YVzoJYlws1?= =?us-ascii?Q?24ej/CtcR/Od+esZyXfv4QkI4g=3D=3D?= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MW2PR2101MB0890.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4723d2e2-d22b-4d31-eee2-08d8963dc43e X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Dec 2020 21:12:10.1515 (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: KSYpX6/QT79wKtZggvKsf5uZUQQq6pc+aWiS1KUcS2fmc1qPfetuZlR0inDvGqmbYRUzT7zAGQZJjO9KlMB8fg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR21MB0829 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_MW2PR2101MB0890E0615CB689A536FD8833EFF41MW2PR2101MB0890_" --_000_MW2PR2101MB0890E0615CB689A536FD8833EFF41MW2PR2101MB0890_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Getting caught up on emails from last week. I wanted to say thanks for the = good catch and the good patch in my absence! Sorry for the oversight. :-/ - Bret From: Laszlo Ersek via groups.io Sent: Wednesday, November 25, 2020 1:17 PM To: Ard Biesheuvel; jejb@linux.ibm.com; devel@edk2.groups.io Cc: Bret Barkelew; Liming Gao (Byosoft= address) Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime pan= ic in ValidateSetVariable() On 11/25/20 22:05, Ard Biesheuvel wrote: > On 11/25/20 9:13 PM, James Bottomley wrote: >> The current variable policy is allocated by AllocatePool(), which is >> boot time only. This means that if you do any variable setting in the >> runtime, the policy has been freed. Ordinarily this isn't detected >> because freed memory is still there, but when you boot the Linux >> kernel, it's been remapped so the actual memory no longer exists in >> the memory map causing a page fault. >> >> Fix this by making it AllocateRuntimePool(). For SMM drivers, the >> platform DSC is responsible for resolving the MemoryAllocationLib >> class to the SmmMemoryAllocationLib instance. In the >> SmmMemoryAllocationLib instance, AllocatePool() and >> AllocateRuntimePool() are implemented identically. Therefore this >> change is a no-op when the RegisterVariablePolicy() function is built >> into an SMM driver. The fix affects runtime DXE drivers only. >> >> Ref: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%= 2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3092&data=3D04%7C01%7Cbr= et.barkelew%40microsoft.com%7C2e91993035204bbd307d08d891878686%7C72f988bf86= f141af91ab2d7cd011db47%7C1%7C0%7C637419358545184416%7CUnknown%7CTWFpbGZsb3d= 8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&= amp;sdata=3DuYtJTRY5RRS5XJ7j%2Bo%2B75qH12ROX9%2FQ4v1GMdUbLk3I%3D&reserv= ed=3D0 >> Signed-off-by: James Bottomley > > Thanks James > > Acked-by: Ard Biesheuvel > >> --- >> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git >> a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c >> b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c >> index 5029ddb96adb..12944ac7ea81 100644 >> --- a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c >> +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c >> @@ -411,7 +411,7 @@ RegisterVariablePolicy ( >> } >> // Reallocate and copy the table. >> - NewTable =3D AllocatePool( NewSize ); >> + NewTable =3D AllocateRuntimePool( NewSize ); >> if (NewTable =3D=3D NULL) { >> return EFI_OUT_OF_RESOURCES; >> } >> > > BTW I wouldn't mind if the whitespace gets fixed up here at merge time. > The coding style all over the VariablePolicy code (that I have investigated) is non-idiomatic for edk2 -- it should have been pointed out during the original patch review sessions. The coding style can also be fixed up retro-actively whole-sale, of course= . In the present patch, James is only sticking with the (non-idiomatic) style that's been part of the VariablePolicy contribution. I'm quite displeased myself with the reams of non-idiomatic coding style in VariablePolicy, but I don't blame that on the contribution -- IMO it should have been caught in review. ( Meta-request: Ard, can you please start signing your emails? (Such as, in "Bye: Ard", not as in cryptographic signing.) It's quite hit-or-miss to know where your emails end; in the present case, I *almost* didn't scroll down to the bottom (because in many other cases, you insert an A-b, don't remove the tail, and add nothing at the bottom, so the reader kind of gets conditioned to stop reading after the A-b, seeing repeatedly how scrolling down to the bottom is a waste). Consistently using a manual signature does away with this problem. Another solution is of course to always strip the tail, when you're done responding. Sorry about this verbiage, I just wanted to have it said. :) ) Thanks, Laszlo --_000_MW2PR2101MB0890E0615CB689A536FD8833EFF41MW2PR2101MB0890_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Getting caught up on emails from last week. I wante= d to say thanks for the good catch and the good patch in my absence!

Sorry for the oversight. :-/

 

- Bret

 

From: Laszlo Ersek via groups.io=
Sent: Wednesday, November 25, 2020 1:17 PM
To: Ard Biesheuvel; <= a href=3D"mailto:jejb@linux.ibm.com"> jejb@linux.ibm.com; devel@edk2= .groups.io
Cc: Bret Barkelew; Liming Gao (Byosoft address)
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runt= ime panic in ValidateSetVariable()

 

On 11/25/20 22:05, A= rd Biesheuvel wrote:
> On 11/25/20 9:13 PM, James Bottomley wrote:
>> The current variable policy is allocated by AllocatePool(), which= is
>> boot time only.  This means that if you do any variable sett= ing in the
>> runtime, the policy has been freed.  Ordinarily this isn't d= etected
>> because freed memory is still there, but when you boot the Linux<= br> >> kernel, it's been remapped so the actual memory no longer exists = in
>> the memory map causing a page fault.
>>
>> Fix this by making it AllocateRuntimePool().  For SMM driver= s, the
>> platform DSC is responsible for resolving the MemoryAllocationLib=
>> class to the SmmMemoryAllocationLib instance. In the
>> SmmMemoryAllocationLib instance, AllocatePool() and
>> AllocateRuntimePool() are implemented identically. Therefore this=
>> change is a no-op when the RegisterVariablePolicy() function is b= uilt
>> into an SMM driver. The fix affects runtime DXE drivers only.
>>
>> Ref: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fbugzil= la.tianocore.org%2Fshow_bug.cgi%3Fid%3D3092&amp;data=3D04%7C01%7Cbret.b= arkelew%40microsoft.com%7C2e91993035204bbd307d08d891878686%7C72f988bf86f141= af91ab2d7cd011db47%7C1%7C0%7C637419358545184416%7CUnknown%7CTWFpbGZsb3d8eyJ= WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&= amp;sdata=3DuYtJTRY5RRS5XJ7j%2Bo%2B75qH12ROX9%2FQ4v1GMdUbLk3I%3D&amp;re= served=3D0
>> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>
> Thanks James
>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>
>> ---
>>   MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c= | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git
>> a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>> b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>> index 5029ddb96adb..12944ac7ea81 100644
>> --- a/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c<= br> >> +++ b/MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c<= br> >> @@ -411,7 +411,7 @@ RegisterVariablePolicy (
>>       }
>>         // Reallocate and copy the = table.
>> -    NewTable =3D AllocatePool( NewSize );
>> +    NewTable =3D AllocateRuntimePool( NewSize );<= br> >>       if (NewTable =3D=3D NULL) {
>>         return EFI_OUT_OF_RESO= URCES;
>>       }
>>
>
> BTW I wouldn't mind if the whitespace gets fixed up here at merge tim= e.
>

The coding style all over the VariablePolicy code (that I have
investigated) is non-idiomatic for edk2 -- it should have been pointed
out during the original patch review sessions.

The coding style can also be fixed up retro-actively whole-sale, of course= .

In the present patch, James is only sticking with the (non-idiomatic)
style that's been part of the VariablePolicy contribution.

I'm quite displeased myself with the reams of non-idiomatic coding style in VariablePolicy, but I don't blame that on the contribution -- IMO it should have been caught in review.

(

Meta-request: Ard, can you please start signing your emails? (Such as,
in "Bye: Ard", not as in cryptographic signing.) It's quite hit-= or-miss
to know where your emails end; in the present case, I *almost* didn't
scroll down to the bottom (because in many other cases, you insert an
A-b, don't remove the tail, and add nothing at the bottom, so the reader kind of gets conditioned to stop reading after the A-b, seeing
repeatedly how scrolling down to the bottom is a waste). Consistently
using a manual signature does away with this problem. Another solution
is of course to always strip the tail, when you're done responding.
Sorry about this verbiage, I just wanted to have it said. :)

)

Thanks,
Laszlo





 

--_000_MW2PR2101MB0890E0615CB689A536FD8833EFF41MW2PR2101MB0890_--