From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail1.protonmail.ch (mail1.protonmail.ch [185.70.40.18]) by mx.groups.io with SMTP id smtpd.web09.1980.1571646600546531092 for ; Mon, 21 Oct 2019 01:30:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@protonmail.com header.s=default header.b=cRxYwO1o; spf=pass (domain: protonmail.com, ip: 185.70.40.18, mailfrom: vit9696@protonmail.com) Date: Mon, 21 Oct 2019 08:29:50 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=default; t=1571646598; bh=A/1UoeDNh3htB+yBnysc03m0ipPFj8WiQFPzzT9bNsI=; h=Date:To:From:Cc:Reply-To:Subject:In-Reply-To:References: Feedback-ID:From; b=cRxYwO1owKSlWFXdGwhBSbHQPlXPD2ZBepWtfNnbzLc2j/Iuozc8kgLgbgvmi+07I fFn2I+JVH8/YOINuFBNueJxRxoHpa0UbSq8VgcYhX6sdt6qPHwsfLwH+Uz6ccke0rK +ekoZJ0p0l68b+GCdHpA98W/t0Wg8Rj6XlbJZjJ4= To: "Yao, Jiewen" From: "Vitaly Cheptsov" Cc: "devel@edk2.groups.io" , "Gao, Liming" , "Wang, Jian J" , "Kinney, Michael D" , Laszlo Ersek , Marvin.Haeuser@outlook.com Reply-To: vit9696 Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions Message-ID: In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F80B961@shsmsx102.ccr.corp.intel.com> References: <20191020130553.42851-1-vit9696@protonmail.com> <20191020130553.42851-2-vit9696@protonmail.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E51F6EB@SHSMSX104.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F80B477@shsmsx102.ccr.corp.intel.com> <6FF18D79-65ED-4C0B-9B03-99C0B5DD539A@protonmail.com> <74D8A39837DF1E4DA445A8C0B3885C503F80B961@shsmsx102.ccr.corp.intel.com> Feedback-ID: p9QuX-L1wMgUm6nrSvNrf8juLupNs0VSnzXGVXuYDxlEahFdWtaedWDMB9zpwGDklGt7kzs1-RBc0cqz327Gcg==:Ext:ProtonMail MIME-Version: 1.0 X-Spam-Status: No, score=-0.7 required=7.0 tests=ALL_TRUSTED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,FREEMAIL_REPLYTO_END_DIGIT autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on mail.protonmail.ch X-Groupsio-MsgNum: 49282 Content-Type: multipart/signed; protocol="application/pgp-signature"; micalg=pgp-sha256; boundary="---------------------8a6f5beecfa7299dd42a52a4f0e2d6a1"; charset=UTF-8 -----------------------8a6f5beecfa7299dd42a52a4f0e2d6a1 Cc: "devel@edk2.groups.io" , "Gao, Liming" , "Wang, Jian J" , "Kinney, Michael D" , Laszlo Ersek , Marvin.Haeuser@outlook.com Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Date: Mon, 21 Oct 2019 11:29:45 +0300 From: vit9696 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F80B961@shsmsx102.ccr.corp.intel.com> Message-Id: Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) References: <20191020130553.42851-1-vit9696@protonmail.com> <20191020130553.42851-2-vit9696@protonmail.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E51F6EB@SHSMSX104.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F80B477@shsmsx102.ccr.corp.intel.com> <6FF18D79-65ED-4C0B-9B03-99C0B5DD539A@protonmail.com> <74D8A39837DF1E4DA445A8C0B3885C503F80B961@shsmsx102.ccr.corp.intel.com> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable safe string constraint assertions To: "Yao, Jiewen" X-Mailer: Apple Mail (2.3445.104.11) Jiewen, Your explanation makes good sense in the context of "This API is NOT desig= n to handle untrusted input.", however, I believe this is not how it is sho= uld 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 Ba= seLib 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 o= nly 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-trivi= al, increases code size, reduces readability, is more prone to errors, make= s 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 inter= faces with different contracts - add a pcd to disable assertions, which will disable the debugging handle= rs and will effectively make these functions behave as runtime checks for u= ntrusted 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 de= fault, 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 =D0=BE=D0=BA=D1=82. 2019 =D0=B3., =D0=B2 11:07, Yao, Jiewen =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >=20 >=20 > Hi Vitaly > We have discussed the ASSERT usage when we added the first version of co= de. >=20 > 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/ >=20 > The default behavior is: > (1) Call Debugger > (2) Print error > (3) Call abort() >=20 > As conclusion, we believe it is *caller's responsibility* to make sure t= he caller inputs right parameter, instead of let callee check and return er= ror. >=20 > In order to catch such error as early as possible, we decide to use ASSE= RT, because it is something never happen. (We still use error handling foll= owed by to handle the release build.) >=20 > This API is NOT design to handle untrusted input. As such, we believe it= is expected ASSERT behavior. >=20 >=20 > 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, inst= ead of callee check that. And ASSERT is a good way to catch the issue as ea= rly as possible. >=20 >=20 >=20 > Thank you > Yao Jiewen >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Vitaly >> Cheptsov via Groups.Io >> Sent: Monday, October 21, 2019 3:28 PM >> To: Yao, Jiewen ; Gao, Liming >> Cc: devel@edk2.groups.io; Wang, Jian J ; Kinney, >> Michael D >> Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable saf= e string >> constraint assertions >>=20 >> Liming, Jiewen, >>=20 >> I am personally fine to resubmit the patch changing the defaults to TRU= E, but >> does it actually make sense in any other environment but some special t= esting >> platform? I cannot imagine any production platform that would need it e= nabled, >> the only use case is to perform analysis on the trusted data usage or s= omething >> similar, as I explained on BZ. >>=20 >> 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 c= ontract >> violation) but also from untrusted sources (external data). Such assert= ions >> cannot be used in production environments, as they may break software, = and >> thus we have to implement code to disable them. >>=20 >> Best regards, >> Vitaly >>=20 >>> 21 =D0=BE=D0=BA=D1=82. 2019 =D0=B3., =D0=B2 7:28, Yao, Jiewen =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0): >>>=20 >>>=20 >>> Hi Mike >>> I remember we have discussed it before. >>>=20 >>> The general concern is that: how many additional PCD we need introduce= , to >> control different ASSERT in different modules ? >>>=20 >>> 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... >>>=20 >>> What is your thought? >>>=20 >>> Thank you >>> Yao Jiewen >>>=20 >>>> -----Original Message----- >>>> From: Gao, Liming >>>> Sent: Monday, October 21, 2019 11:17 AM >>>> To: devel@edk2.groups.io; vit9696@protonmail.com >>>> Cc: Yao, Jiewen ; Wang, Jian J >> ; >>>> Gao, Liming ; Kinney, Michael D >>>> >>>> Subject: RE: [edk2-devel] [PATCH v1 1/1] MdePkg: Add PCD to disable s= afe >> string >>>> constraint assertions >>>>=20 >>>> Include more people. >>>>=20 >>>> Basically, to keep the compatible behavior, >> PcdAssertOnSafeStringConstraints >>>> default value should be TRUE. >>>> The different platform can configure it. >>>>=20 >>>> Thanks >>>> Liming >>>>> -----Original Message----- >>>>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf O= f >>>>> 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 >>>>>=20 >>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054 >>>>>=20 >>>>> 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. >>>>>=20 >>>>> Signed-off-by: Vitaly Cheptsov > >>>>> --- >>>>> 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(-) >>>>>=20 >>>>> 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. >>>>>=20 >>>>>=20 >> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|0x10000 >>>>> 00|UINT64|0x30001015 >>>>>=20 >>>>> + ## Indicates if safe string constraint violation should assert.
>>>>> + # TRUE - Safe string constraint violation causes assertion. >>>>> + # FALSE - Safe string constraint violation does not cause asser= tion.
>>>>> + # @Prompt Enable safe string constraint violation assertions. >>>>> + >>>>>=20 >> gEfiMdePkgTokenSpaceGuid.PcdAssertOnSafeStringConstraints|FALSE|BOOL >>>>> EAN|0x0000002e >>>>> + >>>>> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx= ] >>>>> ## This value is used to set the base address of PCI express hierarc= hy. >>>>> # @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 >>>>>=20 >>>>> [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 >>>>>=20 >>>>> [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 @@ >>>>>=20 >>>>> #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 @@ >>>>>=20 >>>>> #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 decid= ed by >>>>> the value of PcdMaximumGuidedExtractHandler." >>>>>=20 >>>>> +#string >>>>> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_PROM >>>>> PT #language en-US "Enable safe string constraint violation asserti= ons" >>>>> + >>>>> +#string >>>>> STR_gEfiMdePkgTokenSpaceGuid_PcdAssertOnSafeStringConstraints_HELP >>>>> #language en-US "Indicates if safe string constraint violation shoul= d >>>>> assert.

\n" >>>>> + = "TRUE - Safe string >> constraint >>>>> violation causes assertion.
\n" >>>>> + = "FALSE - Safe string >> constraint >>>>> violation does not cause assertion.
" >>>>> + >>>>> #string >>>>> STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_PROMPT >>>>> #language en-US "PCI Express Base Address" >>>>>=20 >>>>> #string STR_gEfiMdePkgTokenSpaceGuid_PcdPciExpressBaseAddress_HELP >>>>> #language en-US "This value is used to set the base address of PCI e= xpress >>>>> hierarchy." >>>>> -- >>>>> 2.21.0 (Apple Git-122) >>>>>=20 >>>>>=20 >>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>>> Groups.io Links: You receive all messages sent to this group. >>>>>=20 >>>>> 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] >>>>> -=3D-=3D-=3D-=3D-=3D-=3D >>>=20 >>=20 >>=20 >>=20 >=20 -----------------------8a6f5beecfa7299dd42a52a4f0e2d6a1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: ProtonMail wsBmBAEBCAAQBQJdrWx6CRBPsoxt7Hy0xQAKCRBPsoxt7Hy0xXeMCAB0wfNU UlfynNX+UnRugIMoqBxgDPjMpyWq1KN+pImQe4tYKMz1Qd+On0qW/xCwKEQA c0eZZUNIfxPshc36OMd1F60z6wIfgNIDS7+kILroLs5Un0QP2E57wT0EZvh/ McRZE+uaNGMNk7xDvr7nSS/dkKmLTik3JOaEjsSswa7ABFPWNyssR5i1hHAA a9WpsJk1zVoVCUJn7yElmiSAHYjkEXye3dHG/6wKAKKc81BcDt2UOewOafu4 KGTzzA07h8+02MppZdAqrdY3jsoqaOk1ns/OuXI3/pqObP9PtP6R8w/boZjy 9xatw61NaA2e372ckJBaYLcrFpc79l+TT4Vm =raeC -----END PGP SIGNATURE----- -----------------------8a6f5beecfa7299dd42a52a4f0e2d6a1--