Hi Laszlo Your analysis below is exactly what the code implemented. Now I believe you and me have different understanding on PI specification, not the UDK implementation. :-)
"Normally this would be a violation of the protocol, because the pointer I'm passing to BootScript->Write() doesn't point to runtime memory or ACPI NVS." Per my understanding, the word in UEFI/PI specification is to limit the caller. So caller need follow UEFI/PI specification to make sure call the function correctly. I do not believe callee need to do any check in this case. It is caller's responsibility. You may also find some examples in UEFI spec, such as TPL. If caller did wrong thing, system may crash. "But the implementation should save the pointer that I pass in, not the pointed-to data." Again, since PI spec does not define format, an implementation can choose to save pointer or save data. Personally, I cannot draw to the conclusion that we must save pointer based on current PI specification. My personal understanding for PI specification is to focus on data. Because if PI specification just need save pointer, there is no need to pass InfomrationLength. This argument is not needed. Anyway, your suggest on "save pointer" is another possible implementation for Boot Script. You may add it in your own package and use the new one in your modules. That is possible, since UDK package supports library override. All in all, since there is different understanding on PI specification, if you want, I suggest we could discuss this topic in PIWG meeting - where we discuss the PI specification. Thank you Yao Jiewen -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Wednesday, December 04, 2013 3:51 AM To: Yao, Jiewen Cc: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] Save State protocol violation in S3SaveState.c (MdeModulePkg) Hi! On 12/03/13 16:03, Yao, Jiewen wrote: > Hi Laszlo > "According to the PI spec, Information is a pointer pointing to > runtime memory or ACPI NVS, and the BootScript record will store the > pointer (ie. not the data pointed-to)." > I agree with you on first part - According to the PI spec, Information > is a pointer pointing to runtime memory or ACPI NVS. > And I hold different view on second part - the BootScript record will > store the pointer (ie. not the data pointed-to) I disagree (and I tested it too). My example code goes like: > EFI_STATUS Status; > EFI_S3_SAVE_STATE_PROTOCOL *BootScript; > STATIC CONST UINT8 Info[] = { 0xDE, 0xAD, 0xBE, 0xEF }; > > Status = gBS->LocateProtocol (&gEfiS3SaveStateProtocolGuid, NULL, > (VOID **) &BootScript); > ASSERT_EFI_ERROR (Status); > > // > // Despite the opcode documentation in the PI spec, the protocol > // implementation embeds a deep copy of the info in the boot script, rather > // than storing just a pointer to runtime or NVS storage. > // > Status = BootScript->Write(BootScript, EFI_BOOT_SCRIPT_INFORMATION_OPCODE, > (UINT32) sizeof Info, > (EFI_PHYSICAL_ADDRESS)(UINTN) &Info); > ASSERT_EFI_ERROR (Status); Normally this would be a violation of the protocol, because the pointer I'm passing to BootScript->Write() doesn't point to runtime memory or ACPI NVS. The implementation behind BootScript->Write() is the BootScriptWrite() function in "MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveState.c". Relevant part: > case EFI_BOOT_SCRIPT_INFORMATION_OPCODE: > VA_START (Marker, OpCode); > Status = BootScriptWriteInformation (Marker); > VA_END (Marker); > break; The BootScriptWriteInformation() function says (still in the same file): > EFI_STATUS > BootScriptWriteInformation ( > IN VA_LIST Marker > ) > { > UINT32 InformationLength; > EFI_PHYSICAL_ADDRESS Information; > > InformationLength = VA_ARG (Marker, UINT32); > Information = VA_ARG (Marker, EFI_PHYSICAL_ADDRESS); > return S3BootScriptSaveInformation (InformationLength, > (VOID*)(UINTN)Information); } The retrieval of "InformationLength" and "Information" from the stack via the outer ellipsis / inner VA_LIST notation is correct. When calling S3BootScriptSaveInformation(), "Information" is converted to a pointer type. I agree that it is not dereferenced (yet), but this is already wrong, because the caller is entitled to pass in a 64-bit EFI_PHYSICAL_ADDRESS, and by converting it to UINTN (and then to VOID*) we could be truncating it, if the DXE phase is 32-bit. Anyway, this is not the main issue; let's suppose that the pointer passed to S3BootScriptSaveInformation() still carries the full EFI_PHYSICAL_ADDRESS. Then, in file "MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c": > RETURN_STATUS > EFIAPI > S3BootScriptSaveInformation ( > IN UINT32 InformationLength, > IN VOID *Information > ) > { > UINT8 Length; > UINT8 *Script; > EFI_BOOT_SCRIPT_INFORMATION ScriptInformation; > > Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + > InformationLength); > > Script = S3BootScriptGetEntryAddAddress (Length); > if (Script == NULL) { > return RETURN_OUT_OF_RESOURCES; > } > // > // Build script data > // > ScriptInformation.OpCode = EFI_BOOT_SCRIPT_INFORMATION_OPCODE; > ScriptInformation.Length = Length; > > > ScriptInformation.InformationLength = InformationLength; > > CopyMem ((VOID*)Script, (VOID*)&ScriptInformation, sizeof > (EFI_BOOT_SCRIPT_INFORMATION)); > CopyMem ((VOID*)(Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION)), > (VOID *) Information, (UINTN) InformationLength); > > SyncBootScript (Script); > > return RETURN_SUCCESS; > > } With my example in mind, the Length local variable is set to sizeof (EFI_BOOT_SCRIPT_INFORMATION) + 4 The "ScriptInformation" structure contains: OpCode = EFI_BOOT_SCRIPT_INFORMATION_OPCODE Length = sizeof (EFI_BOOT_SCRIPT_INFORMATION) + 4 InformationLength = 4 The first CopyMem() call copies this header to the beginning of the new entry in the boot script. The second CopyMem() call copies data into the same new entry, right behind the header. However, it does not copy the address that has been passed in -- it dereferences the address and copies the data. In my case, it copies the four bytes { 0xDE, 0xAD, 0xBE, 0xEF } into the entry. When replaying this entry after resume, the boot script executor calls into the same library, function S3BootScriptExecute(), file "MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptExecute.c": S3BootScriptExecute() BootScriptExecuteInformation() > VOID > BootScriptExecuteInformation ( > IN UINT8 *Script > ) > > { > UINT32 Index; > EFI_BOOT_SCRIPT_INFORMATION Information; > UINT8 *InformationData; > > CopyMem ((VOID*)&Information, (VOID*)Script, > sizeof(EFI_BOOT_SCRIPT_INFORMATION)); > > InformationData = Script + sizeof (EFI_BOOT_SCRIPT_INFORMATION); > DEBUG ((EFI_D_INFO, "BootScriptExecuteInformation - 0x%08x\n", > (UINTN) InformationData)); > > DEBUG ((EFI_D_INFO, "BootScriptInformation: ")); > for (Index = 0; Index < Information.InformationLength; Index++) { > DEBUG ((EFI_D_INFO, "%02x ", InformationData[Index])); > } > DEBUG ((EFI_D_INFO, "\n")); > } The replay function for the INFORMATION opcode first copies out the opcode header from the script entry. It finds the start of the information after the header. Then it dumps the data included in the entry, right after the opcode header. In my case it reads: > S3BootScriptExecute: > TableHeader - 0x9C767000 > TableHeader.TableLength - 0x0000001B > ExecuteBootScript - 9C76700D > EFI_BOOT_SCRIPT_INFORMATION_OPCODE > BootScriptExecuteInformation - 0x9C767014 > BootScriptInformation: DE AD BE EF > ExecuteBootScript - 9C767018 > S3_BOOT_SCRIPT_LIB_TERMINATE_OPCODE > S3BootScriptDone - Success The bytes DE AD BE EF are the contents of my array (saved still before the suspend). They are not the address of the array. > NOTE: PI spec does not say anything on boot script format in memory, > this is implementation specific. I agree. But the implementation should save the pointer that I pass in, not the pointed-to data. > For security reason, BIOS S3 module will try its best to save the all > S3 data to lockbox, so current S3 module implementation purposely save > the information part into boot script and into lockbox later. Ah, that's actually a good reason. It deviates from the spec, but I can understand the idea. > Personally, I believe this implementation still follows PI spec, since > Boot Script format is NOT defined by PI spec. It's not the format but how it works. Consider the following: according to the spec, I should be able to: (1) allocate an area in Acpi NVS memory, (2) zero it out, (3) push the address to the boot script with an INFO opcode, (4) *change* the contents of the area. (5) suspend and resume. According to the spec, after resumption I should see the contents written in (4), not the zeroes written in (2). > "The current code also limits the size of the information to less than > 255 bytes, and makes the InformationLength argument redundant." > I agree with you on first part - The current code also limits the size > of the information to less than 255 bytes. And I hold different view > on second part - makes the InformationLength argument redundant. > > Right, current implementation does have such limitation. If you have > concern, I agree to fix 255 bytes limitation. You would have to change the internal header format to do that. Currently EFI_BOOT_SCRIPT_INFORMATION.Length is just an UINT8. > S3 module does use InformationLength field in current implementation. > Would you please point out why it is "redundant"? Because of these assignments in S3BootScriptSaveInformation(): > Length = (UINT8)(sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength); > ScriptInformation.Length = Length; > ScriptInformation.InformationLength = InformationLength; The Length and the InformationLength fields differ only in a constant. The EFI_BOOT_SCRIPT_INFORMATION header structure is actually correct, it is just not used correctly. The current code does this: +--------+--------+--------+--------+--------+--------+--------+-------------- ... ---------------+ | OpCode | Length | InformationLength | deep copy of user specified data | +--------+--------+--------+--------+--------+--------+--------+-------------- ... ---------------+ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ sizeof (EFI_BOOT_SCRIPT_INFORMATION) bytes | InformationLength bytes ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Length = sizeof (EFI_BOOT_SCRIPT_INFORMATION) + InformationLength bytes The Length and InformationLength fields carry the same information, you can derive one from the other, in either direction. This is why one is redundant. The intended use would look like: +--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+ | OpCode | Length | InformationLength | EFI_PHYSICAL_ADDRESS |----+ +--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+--------+ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^|^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | sizeof (EFI_BOOT_SCRIPT_INFORMATION) bytes | 8 bytes exactly | | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | Length = sizeof (EFI_BOOT_SCRIPT_INFORMATION) + 8 == 7 + 8 == 15 bytes, always | | | +--------- ... -----------+ | | User data in ACPI NVS | <---------------------------------------------------------------------------------------------------------------+ +--------- ... -----------+ ^^^^^^^^^^^^^^^^^^^^^^^^^ InformationLength bytes The replay function should check (using the Length field) that the payload is exactly the size of EFI_PHYSICAL_ADDRESS. Then it should CopyMem() the payload out into an EFI_PHYSICAL_ADDRESS variable, convert that to a pointer, and *dereference* it. And dump the data that's there. Thanks, Laszlo ------------------------------------------------------------------------------ Sponsored by Intel(R) XDK Develop, test and display web and hybrid apps with a single code base. Download it for free now! http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
