From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by mx.groups.io with SMTP id smtpd.web12.684.1662567394216252391 for ; Wed, 07 Sep 2022 09:16:34 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=nkk18ZjZ; spf=pass (domain: kernel.org, ip: 139.178.84.217, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 5A2AB6189F for ; Wed, 7 Sep 2022 16:16:33 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BF2C6C433B5 for ; Wed, 7 Sep 2022 16:16:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662567392; bh=n3XB6OYP8n5HBQNHVfXl2kNpaXnLWz8Kt8u+cqSsTbY=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=nkk18ZjZl44AZ2GsGO/oeL3tr2+m+iuQugSUhFFU1kQ2RvXBEf76Bzu4JmwZZEN9e 5dZxmcbXabmw+edADhsfUTY/dOg7s9oa7nYcxo66rfh8dsv25lzaoNxrjpTryK0Mwy 0dAY1QSxBhd3qbBZusnu+crsQmxBELrZGBH8hx6p0QH4Z5iY/pLMrjlytBJZbdTYS+ 5R7mAA09wmV68Vo4aQNiuXCUTCqgPzZPWT92wshie7OWXQsjopp3uKuDV6BEX5K9w7 txr7lwxGjXasTwX4fYYYz8tmLRc/dGn2QarQMC/mXtcx447SF25PJ3t1OpGQH+FYsm R+OIB1scpExBg== Received: by mail-lf1-f51.google.com with SMTP id bq23so23239278lfb.7 for ; Wed, 07 Sep 2022 09:16:32 -0700 (PDT) X-Gm-Message-State: ACgBeo3g0V+e6n4fYVk6wQdxEyQaZDPTcueq80BOnQzkER7KkUlAoSWS 1+8e8V2XZTKqZMAGXcTwaMFfOqDh/RMd4Us4u1g= X-Google-Smtp-Source: AA6agR7RDylE/JvniQL6CKvH6yBB0V4ABfU2z/rIQl9s/5tO5faJfvplrUxUc4Nifo56hXtz7wRS2udtKP/a5Hvccgs= X-Received: by 2002:a05:6512:2294:b0:494:8dc5:10af with SMTP id f20-20020a056512229400b004948dc510afmr1246972lfu.426.1662567390763; Wed, 07 Sep 2022 09:16:30 -0700 (PDT) MIME-Version: 1.0 References: <3bbe3600b3c7012614a09aedb3543821b33f28c0.1662565486.git.jbrasen@nvidia.com> In-Reply-To: <3bbe3600b3c7012614a09aedb3543821b33f28c0.1662565486.git.jbrasen@nvidia.com> From: "Ard Biesheuvel" Date: Wed, 7 Sep 2022 18:16:19 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] EmbeddedPkg/PrePiMemoryAllocationLib: Check for space on offset allocation To: Jeff Brasen Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org, abner.chang@amd.com, git@danielschaefer.me, quic_llindhol@quicinc.com Content-Type: text/plain; charset="UTF-8" On Wed, 7 Sept 2022 at 17:46, Jeff Brasen wrote: > > Update check for enough space to occur prior to alignment offset > modification. This prevents a case where EfiFreeMemoryTop could be > less than EfiFreeMemoryBottom > > Signed-off-by: Jeff Brasen Thanks for the respin. I have caught up in the mean time. > --- > .../MemoryAllocationLib.c | 53 +++++++++++-------- > 1 file changed, 30 insertions(+), 23 deletions(-) > > diff --git a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c > index 2cc2a71121..9208826565 100644 > --- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c > +++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c > @@ -27,37 +27,44 @@ InternalAllocatePages ( > > Hob.Raw = GetHobList (); > > - // Check to see if on 4k boundary > Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF; > + // > + // Verify that there is sufficient memory to satisfy the allocation and padding prior to updating anything > + // > + if ((Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages * EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) - Offset) < Hob.HandoffInformationTable->EfiFreeMemoryBottom) { > + if (Offset != 0) { > + DEBUG ((DEBUG_ERROR, "Offset applied without enough space\r\n")); > + } else { > + DEBUG ((DEBUG_ERROR, "Out of memory\r\n")); > + } > + > + ASSERT (FALSE); > + return 0; > + } > + > + // Check to see if on 4k boundary > if (Offset != 0) { > // If not aligned, make the allocation aligned. > Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset; > } > I understand how you are trying to stick with the original code as much as possible, but this is all extremely clunky, and I'd prefer we just clean it up, if you don't mind. Would something like the below work for you as well? diff --git a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c index 2cc2a7112197..547117dc13d6 100644 --- a/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c +++ b/EmbeddedPkg/Library/PrePiMemoryAllocationLib/MemoryAllocationLib.c @@ -23,41 +23,36 @@ InternalAllocatePages ( ) { EFI_PEI_HOB_POINTERS Hob; - EFI_PHYSICAL_ADDRESS Offset; + EFI_PHYSICAL_ADDRESS NewTop; Hob.Raw = GetHobList (); - // Check to see if on 4k boundary - Offset = Hob.HandoffInformationTable->EfiFreeMemoryTop & 0xFFF; - if (Offset != 0) { - // If not aligned, make the allocation aligned. - Hob.HandoffInformationTable->EfiFreeMemoryTop -= Offset; - } + NewTop = Hob.HandoffInformationTable->EfiFreeMemoryTop & ~(EFI_PHYSICAL_ADDRESS)EFI_PAGE_MASK; + NewTop -= Pages * EFI_PAGE_SIZE; // // Verify that there is sufficient memory to satisfy the allocation // - if (Hob.HandoffInformationTable->EfiFreeMemoryTop - ((Pages * EFI_PAGE_SIZE) + sizeof (EFI_HOB_MEMORY_ALLOCATION)) < Hob.HandoffInformationTable->EfiFreeMemoryBottom) { - return 0; - } else { - // - // Update the PHIT to reflect the memory usage - // - Hob.HandoffInformationTable->EfiFreeMemoryTop -= Pages * EFI_PAGE_SIZE; - - // This routine used to create a memory allocation HOB a la PEI, but that's not - // necessary for us. - - // - // Create a memory allocation HOB. - // - BuildMemoryAllocationHob ( - Hob.HandoffInformationTable->EfiFreeMemoryTop, - Pages * EFI_PAGE_SIZE, - MemoryType - ); - return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop; + if (NewTop < (Hob.HandoffInformationTable->EfiFreeMemoryBottom + sizeof (EFI_HOB_MEMORY_ALLOCATION))) + { + return NULL; } + + // + // Update the PHIT to reflect the memory usage + // + Hob.HandoffInformationTable->EfiFreeMemoryTop = NewTop; + + // + // Create a memory allocation HOB. + // + BuildMemoryAllocationHob ( + Hob.HandoffInformationTable->EfiFreeMemoryTop, + Pages * EFI_PAGE_SIZE, + MemoryType + ); + + return (VOID *)(UINTN)Hob.HandoffInformationTable->EfiFreeMemoryTop; } /**