Eric,
This is the execution path I'm seeing (all of this is located in
InternalHiiIfrValueAction).
//
// 3. Call ConfigRouting GetAltCfg(ConfigRoute, <ConfigResponse>,
Guid, Name, DevicePath, AltCfgId, AltCfgResp)
// Get the default configuration string according to the default
ID.
//
Status = gHiiConfigRouting->GetAltConfig (
gHiiConfigRouting,
ConfigAltResp,
VarGuid,
VarName,
DevicePath,
(ActionType == ACTION_SET_DEFAUTL_VALUE)
? &DefaultId:NULL, // it can be NULL to get the current setting.
&ConfigResp
);
//
// The required setting can't be found. So, it is not required to be
validated and set.
//
if (EFI_ERROR (Status)) {
Status = EFI_SUCCESS;
goto NextConfigAltResp;
}
After GetAltConfig returns, Status is not EFI_SUCCESS (I can't remember
the exact return code) and ConfigResp is no longer NULL. I don't see this
behavior on OVMF, but I do see it while testing my code on the physical
system I have. It seems like the callee was using ConfigResp for something
while processing the call and didn't set the value back to NULL when it
returned. Again, I believe this is technically correct even if it is
strange.
Regardless, the EFI_ERROR macro evaluates to true, so the code executes
the goto.
NextConfigAltResp:
//
// Free the allocated pacakge buffer and the got ConfigResp string.
//
if (HiiPackageList != NULL) {
FreePool (HiiPackageList);
HiiPackageList = NULL;
}
if (ConfigResp != NULL) {
FreePool (ConfigResp);
ConfigResp = NULL;
}
ConfigResp is not NULL at this point, but it's also not a valid pointer.
Since it is not NULL, it is passed in as an argument to FreePool().
FreePool (
IN VOID *Buffer
)
{
EFI_STATUS Status;
Status = gBS->FreePool (Buffer);
ASSERT_EFI_ERROR (Status);
}
gBS->FreePool() returns EFI_INVALID_PARAMETER, which triggers the assert.
Does that make it more clear? I can easily recreate this if you need more
info.
Chris Barr
"Dong, Eric" <[email protected]>
05/06/2014 08:48 PM
Please respond to
[email protected]
To
"[email protected]" <[email protected]>
cc
"[email protected]" <[email protected]>
Subject
Re: [edk2] [PATCH] MdeModulePkg: Potential invalid free in HiiLib
Christopher,
Base on the spec description, I admit it’s not clear whether ConfigResp is
NULL if error status return. So I check the
gHiiConfigRouting->GetAltConfig function, it will set ConfigResp to NULL
if error status return.
I can’t find a code pass where the ConfigResp will be not NULL and the
return status is error. Can you help to do more investigation and get
more detail about this error?
Thanks,
Eric
From: [email protected] [mailto:[email protected]]
Sent: Monday, May 05, 2014 11:47 PM
To: Dong, Eric
Cc: [email protected]
Subject: Re: [edk2] [PATCH] MdeModulePkg: Potential invalid free in HiiLib
Eric,
I was running a driver with asserts turned on for testing, and was
triggering the "ASSERT_EFI_ERROR (Status);" in FreePool(). I ended up
narrowing it down to this function, with ConfigResp being set to something
other than NULL even though the GetAltCfg function returned bad status.
Since it was triggering an assert I figured I'd submit the patch and see
if it was useful to include.
My first thought was that it was a bug in my workstation's firmware, but
after double-checking with the UEFI specification I'm not sure it is. The
way I read it, there's no real defined state for ConfigResp if the
function fails. It won't be allocated if there's a failure, but the
specification doesn't say it'll remain unchanged. I'm still pretty fresh
when it comes to UEFI, though, so I'm not sure if my interpretation is
right... I could definitely be missing something here.
Chris Barr
"Dong, Eric" <[email protected]>
05/04/2014 08:48 PM
Please respond to
[email protected]
To
"[email protected]" <[email protected]>
cc
"[email protected]" <[email protected]>
Subject
Re: [edk2] [PATCH] MdeModulePkg: Potential invalid free in HiiLib
Christopher,
I think the current code is ok, because before use ConfigResp, we has set
it to NULL, and the ConfigResp will be used to save the return value only
when the return status is success, otherwise this parameter should keep as
NULL.
What do you think? Do you met any issue already?
Thanks,
Eric
From: [email protected] [mailto:[email protected]]
Sent: Thursday, May 01, 2014 1:56 AM
To: [email protected]
Subject: [edk2] [PATCH] MdeModulePkg: Potential invalid free in HiiLib
Dear MdeModulePkg maintainer,
I would like to submit this patch. Do I just post it to this list?
Thank you for your time.
==============================================
MdeModulePkg: Fix potential invalid free in HiiLib
This relates to a previous call to gHiiConfigRouting->GetAltConfig.
The only guarantee the UEFI specification makes about the state of
ConfigResp is that it will be allocated if the function returns
EFI_SUCCESS. Since there is no guarantee that ConfigResp will remain
NULL if GetAltConfig fails, ConfigResp should be reset to NULL so
that the cleanup code for this function does not attempt to call
FreePool() on a potentially invalid pointer.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chris Barr <[email protected]>
---
MdeModulePkg/Library/UefiHiiLib/HiiLib.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
index 09f1ff7..cfadbdb 100644
--- a/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
+++ b/MdeModulePkg/Library/UefiHiiLib/HiiLib.c
@@ -2156,6 +2156,7 @@ InternalHiiIfrValueAction (
// The required setting can't be found. So, it is not required to be
validated and set.
//
if (EFI_ERROR (Status)) {
+ ConfigResp = NULL;
Status = EFI_SUCCESS;
goto NextConfigAltResp;
}------------------------------------------------------------------------------
Is your legacy SCM system holding you back? Join Perforce May 7 to find
out:
• 3 signs your SCM is hindering your productivity
• Requirements for releasing software faster
• Expert tips and advice for migrating your SCM now
http://p.sf.net/sfu/perforce_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Is your legacy SCM system holding you back? Join Perforce May 7 to find
out:
• 3 signs your SCM is hindering your productivity
• Requirements for releasing software faster
• Expert tips and advice for migrating your SCM now
http://p.sf.net/sfu/perforce
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
Is your legacy SCM system holding you back? Join Perforce May 7 to find out:
• 3 signs your SCM is hindering your productivity
• Requirements for releasing software faster
• Expert tips and advice for migrating your SCM now
http://p.sf.net/sfu/perforce
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
