From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.6186.1607038409419139595 for ; Thu, 03 Dec 2020 15:33:29 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=O47gn/Uw; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1607038408; 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=55gFbRPo/KywU8T4+OWFZcwLBBbiwzmbCvEv9iNqs5Q=; b=O47gn/UwkjqeR1tGbSDCSJjqe7L7o7bQpbVcRH4yhB6IJsCmHi5I2rYyeek85lhDtZapEn lkPMpjHvNz+PdH7FyBP/Ipd+UXezL/xa/Uh55vmdatzKQSaqmRRkNEYn9cf7kjWHAWRKXG nE5zAZojiWkLiEuSADt6vGpeD7cD0EA= 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-420-y5Vtbzu1NuSggp-agvdFcQ-1; Thu, 03 Dec 2020 18:33:25 -0500 X-MC-Unique: y5Vtbzu1NuSggp-agvdFcQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 435491005504; Thu, 3 Dec 2020 23:33:24 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-157.ams2.redhat.com [10.36.114.157]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0F5AD5D6AC; Thu, 3 Dec 2020 23:33:22 +0000 (UTC) Subject: Re: [EXTERNAL] Re: [edk2-devel] [PATCH] MdeModulePkg: Fix runtime panic in ValidateSetVariable() To: Ard Biesheuvel , devel@edk2.groups.io, bret.barkelew@microsoft.com, "jejb@linux.ibm.com" Cc: "Liming Gao (Byosoft address)" References: <414b7574bf8249de0cecd16fb422c711feb76e1a.camel@linux.ibm.com> <1b9adc6f-37e3-0a9b-29cc-2c97e8a9e0f5@arm.com> <0f6e92d4-f600-8495-53af-5898a2cdbe2a@redhat.com> From: "Laszlo Ersek" Message-ID: <1d87dccc-8c9f-a5ba-5d3c-146e969a6035@redhat.com> Date: Fri, 4 Dec 2020 00:33:21 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/03/20 12:25, Ard Biesheuvel wrote: > On 12/3/20 11:39 AM, Laszlo Ersek wrote: >> On 12/01/20 22:12, Bret Barkelew via groups.io wrote: >>> 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. :-/ >> >> It happens. It's not great that I didn't get to regression-testing >> earlier during the HFF. It's even less great that I chalked it up to a >> guest kernel issue initially. >> >> What *is* great however is that James kept pushing and fixed it! :) >> >> MemoryAllocationLib is tricky. The abstraction it provides is a bit >> "deceptive", IMO. There's an argument for removing support, between the >> existing instances of this lib class, for *runtime* DXE drivers >> altogether. In runtime DXE drivers, just force programmers to spell out >> gBS, and the desired memory type. In other module types (PEIMs, DXE and >> UEFI drivers, and SMM drivers), there is no ambiguity whether the >> allocated memory survives into OS runtime. (Well, AllocateReservedPool() >> complicates matters a bit, but it's not used frequently.) >> >> Another sign that MemoryAllocationLib is a leaky abstraction is commit >> 1d733f731f968 -- in the SMM instance, AllocatePool() allocates SMRAM, >> but FreePool() is supposed to be able to release both SMRAM and normal >> RAM :( Something seems really fishy there, as one would expect calling >> FreePool() only on what AllocatePool() returned earlier in the *same >> module*. Since AllocatePool() only returns SMRAM in an SMM driver, why >> would FreePool() *have* to release normal RAM? Oof. >> > > I thought traditional SMM drivers could call DXE protocols before end of > DXE, no? Some boot services return pool allocated memory that has to be > freed by the caller (e.g., LocateHandleBuffer()) Yes, and in that case, the SMM driver should release the memory with gBS->FreePool(), not FreePool(). The fact that FreePool() in the SMM instance of MemoryAllocationLib copes, since commit 1d733f731f968, is a misfeature, IMO. (Or even better, the SMM driver should release the memory with SystemTable->BootServices->FreePool(), as having the "gBS" global variable in the SMM module is apparently sometimes seen as a liability; see SmmServicesTableLibConstructor() in "MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.c".) Thanks Laszlo