RE: [PATCH v4 0/2] Add EFI capsule pstore backend support

[Zhuo, Qiuxu]

> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
>
>> It looks like that my test BIOS on Intel Kabylake board interprets it 
>> *pointer" and pack the array, so my patch can work on that board.
>
> Are you running 64-bit firmware on this board? Because the EDK2 side lacks 
> the 'packed' attribute, which means there is 4 bytes of padding after the 
> first member of EFI_CAPSULE_TABLE.

Yes, I was running 64-bit firmware on this board. I confirmed with the BIOS 
engineer that this firmware had already included the " #pragma pack(1)" 
attribute for 1 byte alignment.

> Ah yes, I missed that bit before. So yes, it probably makes most sense to 
> keep your definition, and update the EDK2 side so it works on 64-bit as well.
Hope so.

> I hope we can settle this by the end of this week, and if the clarification 
> aligns with your definition, I will queue the patches as they are.
OK, Thanks! :-)

Re: [PATCH v4 0/2] Add EFI capsule pstore backend support

[Ard Biesheuvel]

On 26 June 2017 at 04:09, Zhuo, Qiuxu  wrote:
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>
>> Actually, no, The issue I raised the last time around was not addressed 
>> anywhere, and is not even mentioned in the commit log.
>>
>> The problem is that there is an ambiguity in the implementation of the 
>> configuration table entries that represent capsule with the 
>> EFI_CAPSULE_PERSIST_ACROSS_RESET and EFI_CAPSULE_POPULATE_SYSTEM_TABLE flags 
>> set.
>>
>> First of all, there is the padding issue:
>>
>> typedef struct {
>>   u32 capsule_array_number;
>>   void *capsule_addr[];
>> } __packed efi_capsule_table_t;
>>
>> This deviates from the implementation used by EDK2 (which does not pack the 
>> array), which means this code is currently broken on 64-bit implementations.
>> Then, there is the issue of the capsule_addr member, > which is described by 
>> the spec as follows:
>>
>> """
>> The EFI System Table entry must point to an array of capsules that contain 
>> the same CapsuleGuid value.
>> """
>>
>> This is open to interpretation, but an 'array of capsules' is not the same 
>> as an 'array of pointers to capsules'.
>>
>> I have reminded the USWG again that this needs to be clarified.
>>
>
> It looks like that my test BIOS on Intel Kabylake board interprets it 
> *pointer" and pack the array, so my patch can work on that board.

Are you running 64-bit firmware on this board? Because the EDK2 side
lacks the 'packed' attribute, which means there is 4 bytes of padding
after the first member of EFI_CAPSULE_TABLE.

> Yes, the spec is ambiguous that  the description[1] in spec treats it as 
> *pointer", and description[2] treats it as *capsule* (header + data ???)
> [1] "Firmware that processes a capsule that has the 
> CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE Flag set in
> its header will coalesce the contents of the capsule from the 
> ScatterGatherList into a contiguous
> buffer and must then place a *pointer* to this coalesced capsule in the EFI 
> System Table after the
> system has been reset. Agents searching for this capsule will look in the 
> EFI_CONFIGURATION_TABLE
> and search for the capsule’s GUID and associated *pointer* to retrieve the 
> data after the reset."
> [2] " The EFI System Table entry must point to an array of *capsules* that 
> contain the same CapsuleGuid value."
>
> I tend to [1] :-), and it seems to me that [2] has a disadvantage that it 
> must have a contiguous physical memory large enough for all capsules.
>

Ah yes, I missed that bit before. So yes, it probably makes most sense
to keep your definition, and update the EDK2 side so it works on
64-bit as well.

>> https://lists.01.org/pipermail/edk2-devel/2017-March/008141.html
>> +  CapsuleTable->CapsuleArraySize = Size - sizeof(EFI_CAPSULE_TABLE);
>I think 'CapsuleTable->CapsuleArraySize = Size - sizeof (U32)' presents 
> the size of capsule array.
>
>> +  for (Index = 0; Index < CapsuleNumber; Index++) {
>> +CopyMem(>Capsule[Index], CapsulePtrCache[Index], 
>> sizeof(EFI_CAPSULE_TABLE));
>> +  }
>  Here it only copies the capsule headers (EFI_CAPSULE_TABLE) to ' 
> CapsuleTable->Capsule[Index]'.
>  Should it also copy capsule data for each capsule here?  Otherwise OS can 
> only get the capsule header I think.
>
> Ard if you get clarification (capsule pointer or capsule header+data in EFI 
> System Table entry) from USWG, would you please notify me and I can update my 
> patch accordingly.
> Thanks!
>

I hope we can settle this by the end of this week, and if the
clarification aligns with your definition, I will queue the patches as
they are.

Thanks,
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v4 0/2] Add EFI capsule pstore backend support

[Zhuo, Qiuxu]

> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
>
> Actually, no, The issue I raised the last time around was not addressed 
> anywhere, and is not even mentioned in the commit log.
>
> The problem is that there is an ambiguity in the implementation of the 
> configuration table entries that represent capsule with the 
> EFI_CAPSULE_PERSIST_ACROSS_RESET and EFI_CAPSULE_POPULATE_SYSTEM_TABLE flags 
> set.
>
> First of all, there is the padding issue:
>
> typedef struct {
>   u32 capsule_array_number;
>   void *capsule_addr[];
> } __packed efi_capsule_table_t;
> 
> This deviates from the implementation used by EDK2 (which does not pack the 
> array), which means this code is currently broken on 64-bit implementations.
> Then, there is the issue of the capsule_addr member, > which is described by 
> the spec as follows:
> 
> """
> The EFI System Table entry must point to an array of capsules that contain 
> the same CapsuleGuid value.
> """
> 
> This is open to interpretation, but an 'array of capsules' is not the same as 
> an 'array of pointers to capsules'.
> 
> I have reminded the USWG again that this needs to be clarified.
> 

It looks like that my test BIOS on Intel Kabylake board interprets it *pointer" 
and pack the array, so my patch can work on that board.   
Yes, the spec is ambiguous that  the description[1] in spec treats it as 
*pointer", and description[2] treats it as *capsule* (header + data ???)
[1] "Firmware that processes a capsule that has the 
CAPSULE_FLAGS_POPULATE_SYSTEM_TABLE Flag set in
its header will coalesce the contents of the capsule from the ScatterGatherList 
into a contiguous
buffer and must then place a *pointer* to this coalesced capsule in the EFI 
System Table after the
system has been reset. Agents searching for this capsule will look in the 
EFI_CONFIGURATION_TABLE
and search for the capsule’s GUID and associated *pointer* to retrieve the data 
after the reset."
[2] " The EFI System Table entry must point to an array of *capsules* that 
contain the same CapsuleGuid value."

I tend to [1] :-), and it seems to me that [2] has a disadvantage that it must 
have a contiguous physical memory large enough for all capsules.  

> For more info, please refer to
> https://lists.01.org/pipermail/edk2-devel/2017-March/008141.html
> +  CapsuleTable->CapsuleArraySize = Size - sizeof(EFI_CAPSULE_TABLE);
   I think 'CapsuleTable->CapsuleArraySize = Size - sizeof (U32)' presents the 
size of capsule array.

> +  for (Index = 0; Index < CapsuleNumber; Index++) {
> +CopyMem(>Capsule[Index], CapsulePtrCache[Index], 
> sizeof(EFI_CAPSULE_TABLE));
> +  }
 Here it only copies the capsule headers (EFI_CAPSULE_TABLE) to ' 
CapsuleTable->Capsule[Index]'.
 Should it also copy capsule data for each capsule here?  Otherwise OS can only 
get the capsule header I think.

Ard if you get clarification (capsule pointer or capsule header+data in EFI 
System Table entry) from USWG, would you please notify me and I can update my 
patch accordingly.
Thanks!

BR
qiuxu

Re: [PATCH v4 0/2] Add EFI capsule pstore backend support

[Ard Biesheuvel]

On 23 June 2017 at 23:03, Ard Biesheuvel  wrote:
> On 23 June 2017 at 20:42, Kees Cook  wrote:
>> On Thu, Jun 22, 2017 at 9:34 AM, Qiuxu Zhuo  wrote:
>>> Change Log v3->v4:
>>>  - Add comment 'the number of config tables' for 'nr_config_table' in efi 
>>> structure
>>>  - Initialize 'efi.nr_config_table' to 0 in default
>>>  - Set 'efi.nr_config_table' to 'efi.systab->nr_tables' in 
>>> drivers/firmware/efi/arm-init.c -> uefi_init()
>>>  - Mark 'efi_capsule_pstore_disable' as __ro_after_init
>>>  - Use timestamp value passed from pstore API rather than using 
>>> get_seconds()
>>>  - Pass page physcial addresses rather than struct page pointers to 
>>> efi_capsule_update()
>>
>> Thanks for the updates!
>>
>> Reviewed-by: Kees Cook 
>>
>
>
> Thanks Qiuxu, Kees.
>
> I will queue these on the EFI -next branch.
>

Actually, no, The issue I raised the last time around was not
addressed anywhere, and is not even mentioned in the commit log.

The problem is that there is an ambiguity in the implementation of the
configuration table entries that represent capsule with the
EFI_CAPSULE_PERSIST_ACROSS_RESET and EFI_CAPSULE_POPULATE_SYSTEM_TABLE
flags set.

First of all, there is the padding issue:

typedef struct {
   u32 capsule_array_number;
   void *capsule_addr[];
} __packed efi_capsule_table_t;

This deviates from the implementation used by EDK2 (which does not
pack the array), which means this code is currently broken on 64-bit
implementations. Then, there is the issue of the capsule_addr member,
which is described by the spec as follows:

"""
The EFI System Table entry must point to an array of capsules that
contain the same CapsuleGuid value.
"""

This is open to interpretation, but an 'array of capsules' is not the
same as an 'array of pointers to capsules'.

I have reminded the USWG again that this needs to be clarified.

For more info, please refer to
https://lists.01.org/pipermail/edk2-devel/2017-March/008141.html

Regards,
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] Add EFI capsule pstore backend support

[Ard Biesheuvel]

On 23 June 2017 at 20:42, Kees Cook  wrote:
> On Thu, Jun 22, 2017 at 9:34 AM, Qiuxu Zhuo  wrote:
>> Change Log v3->v4:
>>  - Add comment 'the number of config tables' for 'nr_config_table' in efi 
>> structure
>>  - Initialize 'efi.nr_config_table' to 0 in default
>>  - Set 'efi.nr_config_table' to 'efi.systab->nr_tables' in 
>> drivers/firmware/efi/arm-init.c -> uefi_init()
>>  - Mark 'efi_capsule_pstore_disable' as __ro_after_init
>>  - Use timestamp value passed from pstore API rather than using get_seconds()
>>  - Pass page physcial addresses rather than struct page pointers to 
>> efi_capsule_update()
>
> Thanks for the updates!
>
> Reviewed-by: Kees Cook 
>


Thanks Qiuxu, Kees.

I will queue these on the EFI -next branch.

-- 
Ard.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] Add EFI capsule pstore backend support

[Kees Cook]

On Thu, Jun 22, 2017 at 9:34 AM, Qiuxu Zhuo  wrote:
> Change Log v3->v4:
>  - Add comment 'the number of config tables' for 'nr_config_table' in efi 
> structure
>  - Initialize 'efi.nr_config_table' to 0 in default
>  - Set 'efi.nr_config_table' to 'efi.systab->nr_tables' in 
> drivers/firmware/efi/arm-init.c -> uefi_init()
>  - Mark 'efi_capsule_pstore_disable' as __ro_after_init
>  - Use timestamp value passed from pstore API rather than using get_seconds()
>  - Pass page physcial addresses rather than struct page pointers to 
> efi_capsule_update()

Thanks for the updates!

Reviewed-by: Kees Cook 

>
> Qiuxu Zhuo (2):
>   efi: Add 'nr_config_table' variable in efi structure
>   eif/capsule-pstore: Add capsule pstore backend
>
>  arch/x86/platform/efi/efi.c   |   1 +
>  drivers/firmware/efi/Kconfig  |  21 ++
>  drivers/firmware/efi/Makefile |   1 +
>  drivers/firmware/efi/arm-init.c   |   4 +-
>  drivers/firmware/efi/capsule-pstore.c | 679 
> ++
>  drivers/firmware/efi/efi.c|   1 +
>  include/linux/efi.h   |   1 +
>  7 files changed, 707 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/firmware/efi/capsule-pstore.c
>
> --
> 2.9.0.GIT
>



-- 
Kees Cook
Pixel Security
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html