Yes of course, it relies on the correctness of the BaseTools. Even the whole
BIOS image relies on the correctness of the BaseTools. ^_^
The ASSERT() you mean is like below? I admit it is an approach to ensure the
correctness of BaseTools.
ASSERT (((UINTN) InternalData & (sizeof (UINT16) - 1)) == 0);
But the ASSERT check maybe a little redundant as it will be in every 16/32/64
PcdSet. Do you think it could be a little more efficient to add ASSERT check
only for the start pointer of uninitialized data(must be 8bytes aligned) as we
found the bug only in the uninitialized data? you can reference the analysis
result about the root cause of the bug in the email I attached in the previous
email thread.
Thanks,
Star
-----Original Message-----
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
Sent: Wednesday, May 6, 2015 1:54 PM
To: Zeng, Star
Cc: edk2-devel@lists.sourceforge.net; Laszlo Ersek; Feng, Bob C
Subject: Re: [edk2] [PATCH] MdeModulePkg: avoid unaligned writes in PcdDxe
driver
On 6 May 2015 at 07:40, Zeng, Star <star.z...@intel.com> wrote:
> InternalData pointer is from PcdDb + Offset, Offset is from LocalTokenNumber
> & PCD_DATABASE_OFFSET_MASK, LocalTokenNumber is from TokenNumber.
>
PCD_DATABASE_OFFSET_MASK does not mask any low bits, and TokenNumber is an
external input to the function that is [indirectly] called from generated code.
So there is nothing in DxePcd itself preventing InternalData from being an
unaligned quantity with respect to its type and size.
> In SetWorker() of Service.c has below code to ensure correct size and then
> correct aligned internal data to be referenced.
>
> if (*Size != DxePcdGetSize (TokenNumber + 1)) {
> return EFI_INVALID_PARAMETER;
> }
>
This checks the size but not the alignment, so it relies on the correctness of
the BaseTools.
But even if you would be able to prove conclusively that InternalData is always
correctly aligned, casting a UINT8* to a pointer to a wider type always needs
either an ASSERT() or a
ReadUnaligned()WriteUnaligned() to be 100% correct.
Regards,
Ard.
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Wednesday, May 6, 2015 1:31 PM
> To: Zeng, Star
> Cc: edk2-devel@lists.sourceforge.net; Laszlo Ersek; Feng, Bob C
> Subject: Re: [edk2] [PATCH] MdeModulePkg: avoid unaligned writes in
> PcdDxe driver
>
> On 6 May 2015 at 05:40, Zeng, Star <star.z...@intel.com> wrote:
>> What is the meaning of "Even if someone decides later on that the PCD
>> database format should use natural alignment for all values, it is still
>> wrong to cast a pointer to void back to a pointer a larger type."?
>>
>
> I am referring to the BaseTools change. Even if those are fixed so
> that only aligned addresses are emitted, we would still need either an
> ASSERT() or my changes at runtime to prevent an uncontrolled crash
> when InternalData inadvertently assumes an unaligned value. My
> preference would be the latter
>
>> About "Since this patch fixes an actual problem on 32-bit ARM running under
>> virtualization, may I suggest that we keep it separate from the discussion
>> regarding the PCD database alignment?", I think the coming BaseTools fix
>> will fix the actual problem you said, we could have you CCed to have a try
>> when the patch will be sent out.
>>
>
> Yes, please. But in the meantime, please consider which runtime change you
> would prefer: an ASSERT() or my changes.
>
> --
> Ard.
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Monday, May 4, 2015 6:23 PM
>> To: Laszlo Ersek
>> Cc: edk2-devel@lists.sourceforge.net
>> Subject: Re: [edk2] [PATCH] MdeModulePkg: avoid unaligned writes in
>> PcdDxe driver
>>
>> On 4 May 2015 at 09:08, Laszlo Ersek <ler...@redhat.com> wrote:
>>> On 04/30/15 14:45, Ard Biesheuvel wrote:
>>>> On 30 April 2015 at 14:31, Laszlo Ersek <ler...@redhat.com> wrote:
>>>>> On 04/30/15 14:04, Ard Biesheuvel wrote:
>>>>>> InternalData may not be aligned to the size of the type we are
>>>>>> writing, so use WriteUnalignedXX() instead.
>>>>>>
>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>>>>> ---
>>>>>> MdeModulePkg/Universal/PCD/Dxe/Service.c | 6 +++---
>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/MdeModulePkg/Universal/PCD/Dxe/Service.c
>>>>>> b/MdeModulePkg/Universal/PCD/Dxe/Service.c
>>>>>> index 9b4701bdd749..f071fc58361b 100644
>>>>>> --- a/MdeModulePkg/Universal/PCD/Dxe/Service.c
>>>>>> +++ b/MdeModulePkg/Universal/PCD/Dxe/Service.c
>>>>>> @@ -1259,15 +1259,15 @@ SetWorker (
>>>>>> break;
>>>>>>
>>>>>> case sizeof(UINT16):
>>>>>> - *((UINT16 *) InternalData) = *((UINT16 *) Data);
>>>>>> + WriteUnaligned16 (InternalData, *((UINT16 *) Data));
>>>>>> break;
>>>>>>
>>>>>> case sizeof(UINT32):
>>>>>> - *((UINT32 *) InternalData) = *((UINT32 *) Data);
>>>>>> + WriteUnaligned32 (InternalData, *((UINT32 *) Data));
>>>>>> break;
>>>>>>
>>>>>> case sizeof(UINT64):
>>>>>> - *((UINT64 *) InternalData) = *((UINT64 *) Data);
>>>>>> + WriteUnaligned64 (InternalData, *((UINT64 *) Data));
>>>>>> break;
>>>>>>
>>>>>> default:
>>>>>>
>>>>>
>>>>> What guarantees that you can dereference Data, for reading?
>>>>> Shouldn't the pattern be:
>>>>>
>>>>> WriteUnalignedXX (InternalData, ReadUnalignedXX (Data));
>>>>>
>>>>> (The reason why this isn't necessary could be obvious from the
>>>>> code, but you surely don't expect me to look at the code, do you.
>>>>> :))
>>>>>
>>>>
>>>> The callers of SetWorker() are currently doing the right thing in
>>>> this
>>>> regard: Data is always aligned to *Size.
>>>> I could add an ASSERT() to enforce that, if you like ...
>>>>
>>>
>>> It's fine like this as far as I'm concerned.
>>>
>>
>> I agree. Even if someone decides later on that the PCD database format
>> should use natural alignment for all values, it is still wrong to cast a
>> pointer to void back to a pointer a larger type. As Laszlo pointed out, this
>> applies not only to InternalData but also to Data, but in the latter case,
>> all call sites can easily be verified to do the right thing.
>>
>> Since this patch fixes an actual problem on 32-bit ARM running under
>> virtualization, may I suggest that we keep it separate from the discussion
>> regarding the PCD database alignment?
>>
>>> Acked-by: Laszlo Ersek <ler...@redhat.com>
>>>
>>
>>
>> Thanks,
>> Ard.
>>
>> ---------------------------------------------------------------------
>> -
>> -------- One dashboard for servers and applications across
>> Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+
>> applications Performance metrics, stats and reports that give you Actionable
>> Insights Deep dive visibility with transaction tracing using APM Insight.
>> http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
