Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions

["Vitaly Cheptsov" <vit9696@protonmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7881 bytes --]

Liming, Jiewen, 

I am personally fine to resubmit the patch changing the defaults to TRUE, but does it actually make sense in any other environment but some special testing platform? I cannot imagine any production platform that would need it enabled, the only use case is to perform analysis on the trusted data usage or something similar, as I explained on BZ.

As for assertions, I would expect that all PCDs we introduce are meant to control unexpected ASSERT behaviour. I.e. where ASSERTs are potentially used to signalise about issues coming not only from trusted sources (usage contract violation) but also from untrusted sources (external data). Such assertions cannot be used in production environments, as they may break software, and thus we have to implement code to disable them.

Best regards,
Vitaly

> 21 окт. 2019 г., в 7:28, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
> 
> 
> Hi Mike
> I remember we have discussed it before.
> 
> The general concern is that: how many additional PCD we need introduce, to control different ASSERT in different modules ?
> 
> We may want to enable *some* assert in some modules, but disable *some other* assert.
> E.g. the assert for linkedlist in base lib is another example...
> 
> What is your thought?
> 
> Thank you
> Yao Jiewen
> 
>> -----Original Message-----
>> From: Gao, Liming <liming.gao@intel.com>
>> Sent: Monday, October 21, 2019 11:17 AM
>> To: devel@edk2.groups.io; vit9696@protonmail.com
>> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
>> Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string
>> constraint assertions
>> 
>> Include more people.
>> 
>> Basically, to keep the compatible behavior, PcdAssertOnSafeStringConstraints
>> default value should be TRUE.
>> The different platform can configure it.
>> 
>> Thanks
>> Liming
>>> -----Original Message-----
>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>>> Vitaly Cheptsov via Groups.Io
>>> Sent: Sunday, October 20, 2019 9:06 PM
>>> To: devel@edk2.groups.io
>>> Subject: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string
>>> constraint assertions
>>> 
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054
>>> 
>>> Runtime data checks are not meant to cause debug assertions
>>> unless explicitly needed by some debug code (thus the PCD)
>>> as this breaks debug builds validating data with BaseLib.
>>> 
>>> Signed-off-by: Vitaly Cheptsov <vit9696@protonmail.com>>
>>> ---
>>> MdePkg/MdePkg.dec                   |  6 ++++++
>>> MdePkg/Library/BaseLib/BaseLib.inf  | 11 ++++++-----
>>> MdePkg/Library/BaseLib/SafeString.c |  4 +++-
>>> MdePkg/MdePkg.uni                   |  6 ++++++
>>> 4 files changed, 21 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
>>> index 3fd7d1634c..dda2cdf401 100644
>>> --- a/MdePkg/MdePkg.dec
>>> +++ b/MdePkg/MdePkg.dec
>>> @@ -2221,6 +2221,12 @@ [PcdsFixedAtBuild,PcdsPatchableInModule]
>>>  # @Prompt Memory Address of GuidedExtractHandler Table.
>>> 
>>> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x10000
>>> 00|UINT64|0x30001015
>>> 
>>> +  ## Indicates if safe string constraint violation should assert.<BR><BR>
>>> +  #   TRUE  - Safe string constraint violation causes assertion.<BR>
>>> +  #   FALSE - Safe string constraint violation does not cause assertion.<BR>
>>> +  # @Prompt Enable safe string constraint violation assertions.
>>> +
>>> gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|FALSE|BOOL
>>> EAN|0x0000002e
>>> +
>>> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>>>  ## This value is used to set the base address of PCI express hierarchy.
>>>  # @Prompt PCI Express Base Address.
>>> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
>>> b/MdePkg/Library/BaseLib/BaseLib.inf
>>> index 3586beb0ab..bc98bc6134 100644
>>> --- a/MdePkg/Library/BaseLib/BaseLib.inf
>>> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
>>> @@ -390,11 +390,12 @@ [LibraryClasses]
>>>  BaseMemoryLib
>>> 
>>> [Pcd]
>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength      ##
>>> SOMETIMES_CONSUMES
>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength     ##
>>> SOMETIMES_CONSUMES
>>> -  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength   ##
>>> SOMETIMES_CONSUMES
>>> -  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask
>>> ## SOMETIMES_CONSUMES
>>> -  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType       ##
>>> SOMETIMES_CONSUMES
>>> +  gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints       ##
>>> SOMETIMES_CONSUMES
>>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength             ##
>>> SOMETIMES_CONSUMES
>>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength            ##
>>> SOMETIMES_CONSUMES
>>> +  gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength          ##
>>> SOMETIMES_CONSUMES
>>> +  gEfiMdePkgTokenSpaceGuid.PcdControlFlowEnforcementPropertyMask
>>> ## SOMETIMES_CONSUMES
>>> +  gEfiMdePkgTokenSpaceGuid.PcdSpeculationBarrierType              ##
>>> SOMETIMES_CONSUMES
>>> 
>>> [FeaturePcd]
>>>  gEfiMdePkgTokenSpaceGuid.PcdVerifyNodeInList  ## CONSUMES
>>> diff --git a/MdePkg/Library/BaseLib/SafeString.c
>>> b/MdePkg/Library/BaseLib/SafeString.c
>>> index 7dc03d2caa..56b5e34a8d 100644
>>> --- a/MdePkg/Library/BaseLib/SafeString.c
>>> +++ b/MdePkg/Library/BaseLib/SafeString.c
>>> @@ -14,7 +14,9 @@
>>> 
>>> #define SAFE_STRING_CONSTRAINT_CHECK(Expression, Status)  \
>>>  do { \
>>> -    ASSERT (Expression); \
>>> +    if (PcdGetBool (PcdAssertOnSafeStringConstraints)) { \
>>> +      ASSERT (Expression); \
>>> +    } \
>>>    if (!(Expression)) { \
>>>      return Status; \
>>>    } \
>>> diff --git a/MdePkg/MdePkg.uni b/MdePkg/MdePkg.uni
>>> index 5c1fa24065..425b66bb43 100644
>>> --- a/MdePkg/MdePkg.uni
>>> +++ b/MdePkg/MdePkg.uni
>>> @@ -287,6 +287,12 @@
>>> 
>>> #string
>>> STR_gEfiMdePkgTokenSpaceGuid_PcdGuidedExtractHandlerTableAddress_H
>>> ELP  #language en-US "This value is used to set the available memory address
>>> to store Guided Extract Handlers. The required memory space is decided by
>>> the value of PcdMaximumGuidedExtractHandler."
>>> 
>>> +#string
>>> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROM
>>> PT  #language en-US "Enable safe string constraint violation assertions"
>>> +
>>> +#string
>>> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP
>>> #language en-US "Indicates if safe string constraint violation should
>>> assert.<BR><BR>\n"
>>> +                                                                                   "TRUE  - Safe string constraint
>>> violation causes assertion.<BR>\n"
>>> +                                                                                   "FALSE - Safe string constraint
>>> violation does not cause assertion.<BR>"
>>> +
>>> #string
>>> STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT
>>> #language en-US "PCI Express Base Address"
>>> 
>>> #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP
>>> #language en-US "This value is used to set the base address of PCI express
>>> hierarchy."
>>> --
>>> 2.21.0 (Apple Git-122)
>>> 
>>> 
>>> -=-=-=-=-=-=
>>> Groups.io Links: You receive all messages sent to this group.
>>> 
>>> View/Reply Online (#49255): https://edk2.groups.io/g/devel/message/49255
>>> Mute This Topic: https://groups.io/mt/35943317/1759384
>>> Group Owner: devel+owner@edk2.groups.io
>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [liming.gao@intel.com]
>>> -=-=-=-=-=-=
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 489 bytes --]