[PATCH v1 0/1] MdePkg: Add PCD to disable safe string constraint assertions

[PATCH v1 0/1] MdePkg: Add PCD to disable safe string constraint assertions

[Vitaly Cheptsov]

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

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2054

Requesting for merge in edk2-stable2019011.

Vitaly Cheptsov (1):
  MdePkg: Add PCD to disable safe string constraint assertions

 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(-)

-- 
2.21.0 (Apple Git-122)


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

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

[Vitaly Cheptsov]

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

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|0x1000000|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|BOOLEAN|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_HELP  #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_PROMPT  #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)


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

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

[Liming Gao]

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]
>-=-=-=-=-=-=

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

[Yao, Jiewen]

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]
> >-=-=-=-=-=-=

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

[Vitaly Cheptsov]

[-- 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 --]

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

[Yao, Jiewen]

Hi Vitaly
We have discussed the ASSERT usage when we added the first version of code.

C11 standard supports runtime violation handler registration.
We investigated the behavior of
(1) MS Visual Studio - http://msdn.microsoft.com/en-us/library/bb288454.aspx
(2) SafeCLib - http://sourceforge.net/projects/safeclib/
(3) Slibc - https://code.google.com/p/slibc/

The default behavior is:
(1) Call Debugger
(2) Print error
(3) Call abort()

As conclusion, we believe it is *caller's responsibility* to make sure the caller inputs right parameter, instead of let callee check and return error.

In order to catch such error as early as possible, we decide to use ASSERT, because it is something never happen. (We still use error handling followed by to handle the release build.)

This API is NOT design to handle untrusted input. As such, we believe it is expected ASSERT behavior.


We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16 process, or if we need ASSERT address alignment for MMIO access.
Per our experience, it is much better to let caller guarantee that, instead of callee check that. And ASSERT is a good way to catch the issue as early as possible.



Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
> Cheptsov via Groups.Io
> Sent: Monday, October 21, 2019 3:28 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>
> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@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
> 
> 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]
> >>> -=-=-=-=-=-=
> >
> 
> 
> 

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

[Vitaly Cheptsov]

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

Jiewen,

Your explanation makes good sense in the context of "This API is NOT design to handle untrusted input.", however, I believe this is not how it is should work or at least this is not how we would like it to behave.

In Unix world there are similar hardened interfaces, for example, strlcpy or strlcat[1]. These interfaces behave quite similar to what you have in BaseLib SafeString, but in the first place they are meant to handle untrusted input. To my experience this situation happens no less often (and in fact much more) than the need to do extra error checking in trusted code, and I expect EDK2 to follow suit. I.e. implement something that can be used not only to check for programmer errors within the module, but also perform the validation of external data. After all, if these were just the assertions, I would have expected for return value to be void not RETURN_STATUS.

If we consider all the options we need to do either of the following:
- reimplement most of the functions in the caller code, which is non-trivial, increases code size, reduces readability, is more prone to errors, makes reviewing more time consuming, etc.
- reimplement all these functions in a separate library, which solves some of the issues, but still increases code size and results in separate interfaces with different contracts
- add a pcd to disable assertions, which will disable the debugging handlers and will effectively make these functions behave as runtime checks for untrusted data, numerous people me included expect them do.

Last suggestion makes most sense to me. Therefore, I believe that even in the situation we have different opinions on whether this should be on by default, we should at least see the benefit for having an option to make this configurable in other projects. In this case I can update the patch in the next days to preserve the original behaviour and resubmit it as a v2.

Best wishes,
Vitaly

[1] https://linux.die.net/man/3/strlcpy

> 21 окт. 2019 г., в 11:07, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
> 
> 
> Hi Vitaly
> We have discussed the ASSERT usage when we added the first version of code.
> 
> C11 standard supports runtime violation handler registration.
> We investigated the behavior of
> (1) MS Visual Studio - http://msdn.microsoft.com/en-us/library/bb288454.aspx
> (2) SafeCLib - http://sourceforge.net/projects/safeclib/
> (3) Slibc - https://code.google.com/p/slibc/
> 
> The default behavior is:
> (1) Call Debugger
> (2) Print error
> (3) Call abort()
> 
> As conclusion, we believe it is *caller's responsibility* to make sure the caller inputs right parameter, instead of let callee check and return error.
> 
> In order to catch such error as early as possible, we decide to use ASSERT, because it is something never happen. (We still use error handling followed by to handle the release build.)
> 
> This API is NOT design to handle untrusted input. As such, we believe it is expected ASSERT behavior.
> 
> 
> We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16 process, or if we need ASSERT address alignment for MMIO access.
> Per our experience, it is much better to let caller guarantee that, instead of callee check that. And ASSERT is a good way to catch the issue as early as possible.
> 
> 
> 
> Thank you
> Yao Jiewen
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
>> Cheptsov via Groups.Io
>> Sent: Monday, October 21, 2019 3:28 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>
>> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@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
>> 
>> 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 --]

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

[Yao, Jiewen]

Hi Vitaly
Before we introduce EDKII version safe string, we did investigate the behavior of below APIs:
1) strcpy - We all know that...
2) strncpy - The problem is that this API cannot guarantee a string is NULL terminated.
3) strlcpy - It is null terminated, but the result is truncated. That data loss might be a problem - https://lwn.net/Articles/507319/ 
4) strcpy_s - Return zero string, if there is violation.

We discussed and decide to follow 4), with a minor update - we just return error without zero the original string.

As such, if you want to use strlcpy, I suggest you had better invent a new API, instead of current StrCpy_S.

Would you please share a real use case on how you use this API verify the untrusted input?
I am very curious on the usage.

Thank you
Yao Jiewen


> -----Original Message-----
> From: vit9696 <vit9696@protonmail.com>
> Sent: Monday, October 21, 2019 4:30 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Wang, Jian J
> <jian.j.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Marvin.Haeuser@outlook.com
> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string
> constraint assertions
> 
> Jiewen,
> 
> Your explanation makes good sense in the context of "This API is NOT design to
> handle untrusted input.", however, I believe this is not how it is should work or
> at least this is not how we would like it to behave.
> 
> In Unix world there are similar hardened interfaces, for example, strlcpy or
> strlcat[1]. These interfaces behave quite similar to what you have in BaseLib
> SafeString, but in the first place they are meant to handle untrusted input. To my
> experience this situation happens no less often (and in fact much more) than the
> need to do extra error checking in trusted code, and I expect EDK2 to follow suit.
> I.e. implement something that can be used not only to check for programmer
> errors within the module, but also perform the validation of external data. After
> all, if these were just the assertions, I would have expected for return value to
> be void not RETURN_STATUS.
> 
> If we consider all the options we need to do either of the following:
> - reimplement most of the functions in the caller code, which is non-trivial,
> increases code size, reduces readability, is more prone to errors, makes
> reviewing more time consuming, etc.
> - reimplement all these functions in a separate library, which solves some of the
> issues, but still increases code size and results in separate interfaces with
> different contracts
> - add a pcd to disable assertions, which will disable the debugging handlers and
> will effectively make these functions behave as runtime checks for untrusted
> data, numerous people me included expect them do.
> 
> Last suggestion makes most sense to me. Therefore, I believe that even in the
> situation we have different opinions on whether this should be on by default, we
> should at least see the benefit for having an option to make this configurable in
> other projects. In this case I can update the patch in the next days to preserve
> the original behaviour and resubmit it as a v2.
> 
> Best wishes,
> Vitaly
> 
> [1] https://linux.die.net/man/3/strlcpy
> 
> > 21 окт. 2019 г., в 11:07, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
> >
> >
> > Hi Vitaly
> > We have discussed the ASSERT usage when we added the first version of code.
> >
> > C11 standard supports runtime violation handler registration.
> > We investigated the behavior of
> > (1) MS Visual Studio - http://msdn.microsoft.com/en-us/library/bb288454.aspx
> > (2) SafeCLib - http://sourceforge.net/projects/safeclib/
> > (3) Slibc - https://code.google.com/p/slibc/
> >
> > The default behavior is:
> > (1) Call Debugger
> > (2) Print error
> > (3) Call abort()
> >
> > As conclusion, we believe it is *caller's responsibility* to make sure the caller
> inputs right parameter, instead of let callee check and return error.
> >
> > In order to catch such error as early as possible, we decide to use ASSERT,
> because it is something never happen. (We still use error handling followed by to
> handle the release build.)
> >
> > This API is NOT design to handle untrusted input. As such, we believe it is
> expected ASSERT behavior.
> >
> >
> > We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16
> process, or if we need ASSERT address alignment for MMIO access.
> > Per our experience, it is much better to let caller guarantee that, instead of
> callee check that. And ASSERT is a good way to catch the issue as early as
> possible.
> >
> >
> >
> > Thank you
> > Yao Jiewen
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
> >> Cheptsov via Groups.Io
> >> Sent: Monday, October 21, 2019 3:28 PM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> >> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@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
> >>
> >> 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]
> >>>>> -=-=-=-=-=-=
> >>>
> >>
> >>
> >> 
> >

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

[Vitaly Cheptsov]

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

Jiewen,

We are aware of all these nuances, and you are right that both of them (strlcpy/strcpy_s) have their downsides. Our opinion is that strlcpy is more practical, but that is a personal choice, and we do nor care much on what is available as long as it is documented.

Our primary issues are not really with string copying, but rather with other interfaces. For example:
- We use AsciiStrToGuid to convert a string to a GUID in the user configuration file, and any intentional or unintentionsl typo there may result in halting the entire app due to length assertion in DEBUG builds.
- We use AsciiStrCats to append messages to the log buffer as long as they fit. We do not care what happens when they stop to fit, for us that means we will have a cut log and a handled error that some messages did not fit. However, in DEBUG builds that once again results in halts.

I am pretty sure there were more, but the use cases are be pretty similar, so you most likely get an idea. We do handle the error where necessary, yet we do not expect the code to halt before we can handle the error.

Best wishes,
Vitaly

В пн, окт. 21, 2019 в 11:51, Yao, Jiewen <jiewen.yao@intel.com> пишет:

> Hi Vitaly
> Before we introduce EDKII version safe string, we did investigate the behavior of below APIs:
> 1) strcpy - We all know that...
> 2) strncpy - The problem is that this API cannot guarantee a string is NULL terminated.
> 3) strlcpy - It is null terminated, but the result is truncated. That data loss might be a problem - https://lwn.net/Articles/507319/
> 4) strcpy_s - Return zero string, if there is violation.
>
> We discussed and decide to follow 4), with a minor update - we just return error without zero the original string.
>
> As such, if you want to use strlcpy, I suggest you had better invent a new API, instead of current StrCpy_S.
>
> Would you please share a real use case on how you use this API verify the untrusted input?
> I am very curious on the usage.
>
> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: vit9696 <vit9696@protonmail.com>
>> Sent: Monday, October 21, 2019 4:30 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>
>> Cc: devel@edk2.groups.io; Gao, Liming <liming.gao@intel.com>; Wang, Jian J
>> <jian.j.wang@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>> Laszlo Ersek <lersek@redhat.com>; Marvin.Haeuser@outlook.com
>> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string
>> constraint assertions
>>
>> Jiewen,
>>
>> Your explanation makes good sense in the context of "This API is NOT design to
>> handle untrusted input.", however, I believe this is not how it is should work or
>> at least this is not how we would like it to behave.
>>
>> In Unix world there are similar hardened interfaces, for example, strlcpy or
>> strlcat[1]. These interfaces behave quite similar to what you have in BaseLib
>> SafeString, but in the first place they are meant to handle untrusted input. To my
>> experience this situation happens no less often (and in fact much more) than the
>> need to do extra error checking in trusted code, and I expect EDK2 to follow suit.
>> I.e. implement something that can be used not only to check for programmer
>> errors within the module, but also perform the validation of external data. After
>> all, if these were just the assertions, I would have expected for return value to
>> be void not RETURN_STATUS.
>>
>> If we consider all the options we need to do either of the following:
>> - reimplement most of the functions in the caller code, which is non-trivial,
>> increases code size, reduces readability, is more prone to errors, makes
>> reviewing more time consuming, etc.
>> - reimplement all these functions in a separate library, which solves some of the
>> issues, but still increases code size and results in separate interfaces with
>> different contracts
>> - add a pcd to disable assertions, which will disable the debugging handlers and
>> will effectively make these functions behave as runtime checks for untrusted
>> data, numerous people me included expect them do.
>>
>> Last suggestion makes most sense to me. Therefore, I believe that even in the
>> situation we have different opinions on whether this should be on by default, we
>> should at least see the benefit for having an option to make this configurable in
>> other projects. In this case I can update the patch in the next days to preserve
>> the original behaviour and resubmit it as a v2.
>>
>> Best wishes,
>> Vitaly
>>
>> [1] https://linux.die.net/man/3/strlcpy
>>
>> > 21 окт. 2019 г., в 11:07, Yao, Jiewen <jiewen.yao@intel.com> написал(а):
>> >
>> >
>> > Hi Vitaly
>> > We have discussed the ASSERT usage when we added the first version of code.
>> >
>> > C11 standard supports runtime violation handler registration.
>> > We investigated the behavior of
>> > (1) MS Visual Studio - http://msdn.microsoft.com/en-us/library/bb288454.aspx
>> > (2) SafeCLib - http://sourceforge.net/projects/safeclib/
>> > (3) Slibc - https://code.google.com/p/slibc/
>> >
>> > The default behavior is:
>> > (1) Call Debugger
>> > (2) Print error
>> > (3) Call abort()
>> >
>> > As conclusion, we believe it is *caller's responsibility* to make sure the caller
>> inputs right parameter, instead of let callee check and return error.
>> >
>> > In order to catch such error as early as possible, we decide to use ASSERT,
>> because it is something never happen. (We still use error handling followed by to
>> handle the release build.)
>> >
>> > This API is NOT design to handle untrusted input. As such, we believe it is
>> expected ASSERT behavior.
>> >
>> >
>> > We have other debate, such as if we need ASSERT 2 bytes alignment CHAR16
>> process, or if we need ASSERT address alignment for MMIO access.
>> > Per our experience, it is much better to let caller guarantee that, instead of
>> callee check that. And ASSERT is a good way to catch the issue as early as
>> possible.
>> >
>> >
>> >
>> > Thank you
>> > Yao Jiewen
>> >
>> >> -----Original Message-----
>> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Vitaly
>> >> Cheptsov via Groups.Io
>> >> Sent: Monday, October 21, 2019 3:28 PM
>> >> To: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> >> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@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
>> >>
>> >> 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: Type: text/html, Size: 20849 bytes --]

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

[Laszlo Ersek]

On 10/21/19 11:58, Vitaly Cheptsov via Groups.Io wrote:
> Jiewen,
>
> We are aware of all these nuances, and you are right that both of them
> (strlcpy/strcpy_s) have their downsides. Our opinion is that strlcpy
> is more practical, but that is a personal choice, and we do nor care
> much on what is available as long as it is documented.
>
> Our primary issues are not really with string copying, but rather with
> other interfaces. For example:
> - We use AsciiStrToGuid to convert a string to a GUID in the user
>   configuration file, and any intentional or unintentionsl typo there
>   may result in halting the entire app due to length assertion in
>   DEBUG builds.
> - We use AsciiStrCats to append messages to the log buffer as long as
>   they fit. We do not care what happens when they stop to fit, for us
>   that means we will have a cut log and a handled error that some
>   messages did not fit. However, in DEBUG builds that once again
>   results in halts.
>
> I am pretty sure there were more, but the use cases are be pretty
> similar, so you most likely get an idea. We do handle the error where
> necessary, yet we do not expect the code to halt before we can handle
> the error.

(Since I've been CC'd somewhere mid-thread, I'll offer an opinion...)

- For copying strings, I'm with Ulrich Drepper:

  http://www.sourceware.org/ml/libc-alpha/2000-08/msg00061.html

  "Every program which is handling strings has to know how long they
  are."

  Because I share that opinion, I've never considered the "safe string"
  functions extremely helpful.

- For formatting strings, and especially for parsing strings, I tend to
  consider functions that cannot deal, by design, with untrusted input,
  more or less useless.

When I was initially working on OvmfPkg/Library/QemuBootOrderLib, and
had to parse some hex strings (as parts of OpenFirmware device paths
exported by QEMU), I believe I looked for suitable utility functions in
edk2 elsewhere. Ultimately I wrote my own ParseUnitAddressHexList()
function.

A note specific to my use case (= guest firmware): in theory, the host
has complete control over the guest, so one might claim that the guest
should never sanity check input from the host (= the guest should always
blindly trust input from the host). That has never sat well with me,
although I couldn't really tell why; it's just always felt wrong. Later,
my concern has been vindicated in two ways: first, in the ACPI
linker/loader client, those sanity checks helped identify QEMU issues
(not malicious intent for sure, but mistakes nonetheless); and then, we
now have SEV (Secure Encrypted Virtualization), where host software does
not have full control over the guest.

IMO all parser functions (exposed as general utility functions) should
expect malicious input by default. (Another example I've been involved
with: the base64 decoder.)

Thanks
Laszlo