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.web09.23742.1662379287998861099 for ; Mon, 05 Sep 2022 05:01:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=JmZ8qCPI; 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 5F84161237 for ; Mon, 5 Sep 2022 12:01:27 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id C46EFC43470 for ; Mon, 5 Sep 2022 12:01:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1662379286; bh=HqZ28qMzd9uzISXcz5ejQWntk+B8+UOgcunm/d15mmI=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=JmZ8qCPIBIq4OLR4oeEuw5aK8l7jjMi2/BrmXHn6rLDYKsvYGgmZDrrwCoNwG2WNR 3S0pC8n4VN5KpIAXA55jfzWVzSv8wfdE2ec2aaQvV2PSftbGCl2OATnN3woa0kT9ol p7YXvFtue8rpKnKS8WZWPNfDWQjqGQr/Sxgbk8j/7Mx6p07O9UQ/nzuyAoMTKLOhL7 wSHM51KYUgOcMJXrL+8WobhmzNsu+pQoF4mZ4osOBtmJEapD/3mObEX3337QIkfbo6 KnDD31pZYrkgzTwTKpfGuA7rGeTEsnddnpS8Muum/Ep30nmdnPWpJC+9WgYaLzU1wn K5WYuy8ziGdIw== Received: by mail-lf1-f51.google.com with SMTP id bq23so12814406lfb.7 for ; Mon, 05 Sep 2022 05:01:26 -0700 (PDT) X-Gm-Message-State: ACgBeo2mbcKdy2wiKx9TFn34eRX3wQBxc2JDmr3fbUCn93gcd2yuWcc0 EvSGGe719LGBwDoyEqgxdfsRp7le6RHYWjdHF3A= X-Google-Smtp-Source: AA6agR4LNr1AhmQOlOT+6dllEe6rgZJ7/chrd1OhqUCcMfDL54wjPGoJzO6ZyL0CxP3zrwSc6umR/4sLzuPrX9F4JSU= X-Received: by 2002:a05:6512:402:b0:494:78bb:f538 with SMTP id u2-20020a056512040200b0049478bbf538mr9949700lfk.637.1662379284830; Mon, 05 Sep 2022 05:01:24 -0700 (PDT) MIME-Version: 1.0 References: <20220818195842.3318813-1-dimitrije.pavlov@arm.com> In-Reply-To: <20220818195842.3318813-1-dimitrije.pavlov@arm.com> From: "Ard Biesheuvel" Date: Mon, 5 Sep 2022 14:01:13 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v1 1/1] OvmfPkg/PlatformDxe: Handle all requests in ExtractConfig and RouteConfig To: Dimitrije Pavlov Cc: devel@edk2.groups.io, Ard Biesheuvel , Jiewen Yao , Liming Gao , Sunny Wang , Jeff Booher-Kaeding , Samer El-Haj-Mahmoud Content-Type: text/plain; charset="UTF-8" On Thu, 18 Aug 2022 at 21:58, Dimitrije Pavlov wrote: > > Per the UEFI specification, if the Request argument in > EFI_HII_CONFIG_ACCESS_PROTOCOL.ExtractConfig() is NULL or does not contain > any request elements, the implementation should return all of the settings > being abstracted for the particular ConfigHdr reference. > > The current implementation returns EFI_INVALID_PARAMETER if Request is > NULL or does not contain any request elements. Instead, construct > a new ConfigRequest to handle these cases per the specification. > > In addition, per the UEFI specification, if the Configuration argument in > EFI_HII_CONFIG_ACCESS_PROTOCOL.RouteConfig() has a ConfigHdr that > specifies a non-existing target, the implementation should return > EFI_NOT_FOUND. > > The current implementation returns EFI_INVALID_PARAMETER if Configuration > has a non-existing target in ConfigHdr. Instead, perform a check and > return EFI_NOT_FOUND in this case. > > Cc: Ard Biesheuvel > Cc: Jiewen Yao > Cc: Liming Gao > Cc: Sunny Wang > Cc: Jeff Booher-Kaeding > Cc: Samer El-Haj-Mahmoud > > Signed-off-by: Dimitrije Pavlov Was this issue caught in some kind of testing/validation? > --- > OvmfPkg/PlatformDxe/PlatformConfig.h | 2 + > OvmfPkg/PlatformDxe/Platform.c | 115 +++++++++++++++++++- > OvmfPkg/PlatformDxe/PlatformConfig.c | 2 +- > 3 files changed, 116 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/PlatformDxe/PlatformConfig.h b/OvmfPkg/PlatformDxe/PlatformConfig.h > index 902c9b2ce043..5d9b457b1b4b 100644 > --- a/OvmfPkg/PlatformDxe/PlatformConfig.h > +++ b/OvmfPkg/PlatformDxe/PlatformConfig.h > @@ -50,4 +50,6 @@ PlatformConfigLoad ( > #define PLATFORM_CONFIG_F_GRAPHICS_RESOLUTION BIT0 > #define PLATFORM_CONFIG_F_DOWNGRADE BIT63 > > +extern CHAR16 mVariableName[]; > + > #endif // _PLATFORM_CONFIG_H_ > diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c > index a6d459f3dfd7..0e32c6e76037 100644 > --- a/OvmfPkg/PlatformDxe/Platform.c > +++ b/OvmfPkg/PlatformDxe/Platform.c > @@ -108,6 +108,11 @@ STATIC EFI_EVENT mGopEvent; > // > STATIC VOID *mGopTracker; > > +// > +// The driver image handle, used to obtain the device path for . > +// > +STATIC EFI_HANDLE mImageHandle; > + > // > // Cache the resolutions we get from the GOP. > // > @@ -229,6 +234,10 @@ ExtractConfig ( > { > MAIN_FORM_STATE MainFormState; > EFI_STATUS Status; > + EFI_STRING ConfigRequestHdr; > + EFI_STRING ConfigRequest; > + UINTN Size; > + BOOLEAN AllocatedRequest; > > DEBUG ((DEBUG_VERBOSE, "%a: Request=\"%s\"\n", __FUNCTION__, Request)); > > @@ -236,18 +245,73 @@ ExtractConfig ( > return EFI_INVALID_PARAMETER; > } > > + ConfigRequestHdr = NULL; > + ConfigRequest = NULL; > + Size = 0; > + AllocatedRequest = FALSE; > + > + // > + // Check if matches the GUID and name > + // > + *Progress = Request; > + if ((Request != NULL) && > + !HiiIsConfigHdrMatch ( > + Request, > + &gOvmfPlatformConfigGuid, > + mVariableName > + ) > + ) > + { > + return EFI_NOT_FOUND; > + } > + > Status = PlatformConfigToFormState (&MainFormState); > if (EFI_ERROR (Status)) { > - *Progress = Request; > return Status; > } > > + if ((Request == NULL) || (StrStr (Request, L"OFFSET") == NULL)) { > + // > + // Request has no , so construct full request string. > + // Allocate and fill a buffer large enough to hold > + // followed by "&OFFSET=0&WIDTH=WWWWWWWWWWWWWWWW" followed by a > + // null terminator. > + // > + ConfigRequestHdr = HiiConstructConfigHdr ( > + &gOvmfPlatformConfigGuid, > + mVariableName, > + mImageHandle > + ); > + if (ConfigRequestHdr == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + Size = (StrLen (ConfigRequestHdr) + 32 + 1) * sizeof (CHAR16); > + ConfigRequest = AllocateZeroPool (Size); > + AllocatedRequest = TRUE; > + if (ConfigRequest == NULL) { > + FreePool (ConfigRequestHdr); > + return EFI_OUT_OF_RESOURCES; > + } > + > + UnicodeSPrint ( > + ConfigRequest, > + Size, > + L"%s&OFFSET=0&WIDTH=%016LX", > + ConfigRequestHdr, > + sizeof MainFormState > + ); > + FreePool (ConfigRequestHdr); > + } else { > + ConfigRequest = Request; > + } > + > // > // Answer the textual request keying off the binary form state. > // > Status = gHiiConfigRouting->BlockToConfig ( > gHiiConfigRouting, > - Request, > + ConfigRequest, > (VOID *)&MainFormState, > sizeof MainFormState, > Results, > @@ -265,6 +329,33 @@ ExtractConfig ( > DEBUG ((DEBUG_VERBOSE, "%a: Results=\"%s\"\n", __FUNCTION__, *Results)); > } > > + // > + // If we used a newly allocated ConfigRequest, update Progress to point to > + // original Request instead of ConfigRequest. > + // > + if (Request == NULL) { > + *Progress = NULL; > + } else if (StrStr (Request, L"OFFSET") == NULL) { > + if (EFI_ERROR (Status)) { > + // > + // Since we constructed ConfigRequest, failure can only occur if there > + // is not enough memory. In this case, we point Progress to the first > + // character of Request. > + // > + *Progress = Request; > + } else { > + // > + // In case of success, we point Progress to the null terminator of > + // Request. > + // > + *Progress = Request + StrLen (Request); > + } > + } > + > + if (AllocatedRequest) { > + FreePool (ConfigRequest); > + } > + > return Status; > } > > @@ -348,6 +439,21 @@ RouteConfig ( > return EFI_INVALID_PARAMETER; > } > > + // > + // Check if matches the GUID and name > + // > + *Progress = Configuration; > + if ((Configuration != NULL) && > + !HiiIsConfigHdrMatch ( > + Configuration, > + &gOvmfPlatformConfigGuid, > + mVariableName > + ) > + ) > + { > + return EFI_NOT_FOUND; > + } > + > // > // the "read" step in RMW > // > @@ -866,6 +972,11 @@ PlatformInit ( > return Status; > } > > + // > + // Save the driver image handle. > + // > + mImageHandle = ImageHandle; > + > // > // Publish the HII package list to HII Database. > // > diff --git a/OvmfPkg/PlatformDxe/PlatformConfig.c b/OvmfPkg/PlatformDxe/PlatformConfig.c > index e202ac5b4798..f5ac2d0609ff 100644 > --- a/OvmfPkg/PlatformDxe/PlatformConfig.c > +++ b/OvmfPkg/PlatformDxe/PlatformConfig.c > @@ -21,7 +21,7 @@ > // > // Name of the UEFI variable that we use for persistent storage. > // > -STATIC CHAR16 mVariableName[] = L"PlatformConfig"; > +CHAR16 mVariableName[] = L"PlatformConfig"; > > /** > Serialize and persistently save platform configuration. > -- > 2.37.2 >