From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.ispras.ru (mail.ispras.ru [83.149.199.45]) by mx.groups.io with SMTP id smtpd.web10.7517.1583932164561685893 for ; Wed, 11 Mar 2020 06:09:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.45, mailfrom: cheptsov@ispras.ru) Received: from [10.0.187.63] (unknown [217.114.218.20]) by mail.ispras.ru (Postfix) with ESMTPSA id A8F89725C1; Wed, 11 Mar 2020 16:09:17 +0300 (MSK) Mime-Version: 1.0 (1.0) Subject: Re: Disabling safe string constraint assertions From: cheptsov@ispras.ru In-Reply-To: <5F8859C7-5776-4DF0-99F5-1D0BC56B50C4@apple.com> Cc: devel@edk2.groups.io Date: Wed, 11 Mar 2020 16:09:14 +0300 Message-Id: <318F875F-3C9A-4236-841B-5BB9908CC139@ispras.ru> References: <5F8859C7-5776-4DF0-99F5-1D0BC56B50C4@apple.com> To: Andrew Fish , Laszlo Ersek , Mike Kinney , =?utf-8?Q?Marvin_H=C3=A4user?= , "Gao, Liming" , "Gao, Zhichao" X-Mailer: iPad Mail (17D50) Content-Type: multipart/alternative; boundary=Apple-Mail-DFBB40DE-33CD-4AD8-82DF-11ABD6884DB4 Content-Transfer-Encoding: 7bit --Apple-Mail-DFBB40DE-33CD-4AD8-82DF-11ABD6884DB4 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hi everyone, So, I believe that by now we mostly agreed to let the original proposition l= and as a short-term solution. We end up with: 1. A PCD condition within SAFE_STRING_COSTRAINT_CHECK macro. 2. Make this condition evaluate to TRUE by default (i.e. ASSERT). 3. Update documentation for BaseLib functions to include the information abo= ut this behaviour. The only thing in question is whether this should be a separate PCD or an ex= tra bit in PcdDebugPropertyMask. I believe that we almost agreed on two thin= gs: 1. Adding an extra bit to PcdDebugPropertyMask is cleaner. 2. Extending DebugLib interface with a new function is not a good idea. Therefore I suggest: 1.Add #define DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40. 2. Create header-only macros to replace functions like DebugAssertEnabled. W= e can then use these macros in new code and deprecate the original functions= . 3. Enable DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED bit in MdePkg by default.= I will submit the new version of the patch soon unless there is an immediate= opposing opinion. Best wishes, Vitaly >> On 4 Mar 2020, at 21:57, Andrew Fish wrote: > =EF=BB=BF >=20 >> On Mar 4, 2020, at 10:18 AM, Laszlo Ersek wrote: >>=20 >>> On 03/04/20 18:53, Andrew Fish wrote: >>>=20 >>>=20 >>>> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek wrote: >>>>=20 >>>>> On 03/03/20 22:12, Vitaly Cheptsov wrote: >>>>> Hello, >>>>>=20 >>>>> I am creating a new thread for this issue, as for some reason half of m= y messages do not display in the web-interface and some of e-mail clients in= the previous one. It seems that somebody sent broken HTML code to groups.io= , and it did not like to be quoted. It will be quite nic= e if all the messages sent are plain-text to avoid such issues in the future= . >>>>>=20 >>>>> Back to the issue, we want to make constraint assertions (SAFE_STRING_= CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as= they break our ability to use some of BaseLib interfaces to verify untruste= d user data in UEFI Applications. The background of this problem is covered i= n a bugzilla[1], and was also extensively discussed in the last proposed pat= ch discussion thread[2]. We want the solution to the problem to land in the n= ext stable tag: edk2-stable202005, and can submit the patch. >>>>>=20 >>>>> Currently there is no clear decision on what approach to take, as ther= e are three different proposals to implement: >>>>>=20 >>>>> 1) Andrew Fish=E2=80=99s approach with C11-like constraint handler[3].= >>>>> 2) Marvin H=C3=A4user=E2=80=99s approach with return codes and constra= int assertions being moved to the caller side[4]. >>>>> 3) My approach suggesting DebugLib interface simplification, permittin= g us to resolve the problem and keep backwards compatibility[5]. >>>>>=20 >>>>> My personal opinion is that: >>>>>=20 >>>>> =E2=80=94 Marvin=E2=80=99s way is very interesting, but it would requi= re implementing code on the caller side basically for every call, and this w= ould requires extra programming effort. It may also be uneasy to teach peopl= e how to do this right, and this does not strictly provide a good solution f= or situations, where nesting is more than 2 (i.e. when user code calls libra= ry code, which itself calls library code). Because of these downsides I am a= fraid it is impractical to adopt it in EDK II. >>>>> =E2=80=94 Andrew=E2=80=99s way is reasonable from the C standard point= of view, but it does not work quite right with reentrancy and I am not posi= tive we need the dynamic handling, especially because of DEBUG/RELEASE build= confusion. I explained this in my previous e-mail, which I attached to the e= nd of this message as quote, since groups.io ate the pre= vious send. Other than that, I would say that I can implement this approach i= f there is no other option. >>>>> =E2=80=94 My own approach makes sense, as it reduces the amount of cod= e in every DebugLib and actually simplifies the development in the future. I= believe currently this is the most reasonable route we can take, and sugges= t other parties to consider it. >>>>>=20 >>>>> Since the three of us each have their own suggestions, I ask Mike and o= thers to rejoin our discussions and give their opinions quickly, so that we c= an choose, perhaps vote, and proceed with the implementation. Thanks for you= r dedication. >>>>=20 >>>> I support the original proposal given in: >>>>=20 >>>> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054#c0 >>>>=20 >>>> also known as the v3 posting: >>>>=20 >>>> * [edk2-devel] [PATCH v3 1/1] >>>> MdePkg: Add PCD to disable safe string constraint assertions >>>>=20 >>>> https://edk2.groups.io/g/devel/message/52838 >>>> http://mid.mail-archive.com/20200103171242.63839-2-vit9696@protonmail.c= om >>>>=20 >>>> With the following tweak: I think we should rather introduce a new bit >>>> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD. >>>=20 >>> Laszlo, >>>=20 >>> +1 >>>=20 >>>> (Note that both PcdDebugPropertyMask and the propsed new PCD support >>>> precisely the following PCD access methods: PcdsFixedAtBuild, >>>> PcdsPatchableInModule.) >>>>=20 >>>> I don't feel too strongly about this part (i.e., whether we introduce a= >>>> new PCD, or a new bit in PcdDebugPropertyMask), as long as we make sure= >>>> that the access method can *never* become dynamic. >>>>=20 >>>> I do prefer quite strongly the original proposal, at a higher level. >>>>=20 >>>> Here's why I support the original proposal: >>>>=20 >>>> - In a brand new code base (or brand new set of APIs), I would fight >>>> tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up t= o >>>> the caller to check return values. Also, additional wrapper >>>> functions/macros can be offered *on top* of the base APIs that >>>> internalize the ASSERT()s for special use cases. >>>>=20 >>>> But this ship has sailed, for edk2. >>>=20 >>> Crazy idea but we could add versions of APIs that don't do the checking a= s new APIs? That would make life easier for untrusted code, Runtime Drivers,= code running on the AP, and other places that the ASSERTs could have side e= ffects.=20 >>=20 >> I'm not sure I understand "new APIs" correctly. >>=20 >> If it's an entirely new set of APIs, then I think it's not good for >> Vitaly -- I understand Vitaly wants to use the existent codebase without >> much churn, just with the ASSERTs removed. >>=20 >> If you mean a new library instance, that's different. If the package >> owners do not mind the code duplication that's inherent in creating such >> an alternative library instance, then sure, it can work for me. >>=20 >=20 > Laszlo, >=20 > Sorry the new APIs would be in addition, and the simplest way to cleanly s= olve your concern that I share about the caller not having control.=20 >=20 > I don't want a long term goal to block getting the short term features nee= ded by the community. >=20 > Thanks, >=20 > Andrew Fish >=20 >>>=20 >>>> - I'm unfriendly towards callbacks. They make the behavior of code >>>> implicit rather than explicit, and implicit is more difficult to reason= >>>> about. IMO callbacks should be considered a last resort only. >>>=20 >>> Not my 1st choice either as I agree with your first point about given th= e caller control. I was just wanted to go through the exercise of what it wo= uld take to make it like C11, and give the caller control.=20 >>>=20 >>>> Just my two cents, because I've been asked to comment. >>>=20 >>> Thanks for helping out. >>=20 >> Thanks! >> Laszlo --Apple-Mail-DFBB40DE-33CD-4AD8-82DF-11ABD6884DB4 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable
Hi everyone,=

So, I believe that by now w= e mostly agreed to let the original proposition land as a short-term solutio= n. We end up with:

1. A PCD= condition within SAFE_STRING_COSTRAINT_CHECK macro.
2= . Make this condition evaluate to TRUE by default (i.e. ASSERT).
3. Update documentation for BaseLib functions to include the info= rmation about this behaviour.

The only thing in question is whether this should be a separate PCD or an= extra bit in PcdDebugPropertyMask. I believe that we almost agreed on two t= hings:

1. Adding an extra b= it to PcdDebugPropertyMask is cleaner.
2. Extending De= bugLib interface with a new function is not a good idea.

Therefore I suggest:
1.Add #define DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABL= ED 0x40.
2. Create header-only macros to replace funct= ions like DebugAssertEnabled. We can then use these macros in new code and d= eprecate the original functions.
3. Enable DEBUG_PROPERTY_ASSE= RT_CONSTRAINT_ENABLED bit in MdePkg by default.

=
I will submit the new version of the patch soon unless there is a= n immediate opposing opinion.

Best wi= shes,
Vitaly

On 4 Mar 2020, at 21:57, Andrew Fish <afish@apple.com> wr= ote:

=EF= =BB=BF

<= div class=3D"">On Mar 4, 2020, at 10:18 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 03/04/20 18:53, Andre= w Fish wrote:


On Mar 4, 2020,= at 5:33 AM, Laszlo Ersek <lersek@redhat.com> wrote:

On 03/03/20 2= 2:12, Vitaly Cheptsov wrote:
Hello,

I am creating a new thread for this i= ssue, as for some reason half of my messages do not display in the web-inter= face and some of e-mail clients in the previous one. It seems that somebody s= ent broken HTML code to groups.io <http://groups.io/>, an= d it did not like to be quoted. It will be quite nice if all the messages se= nt are plain-text to avoid such issues in the future.

Back to the issue, we want to make constraint assertions (SAFE_STRING= _CONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, a= s they break our ability to use some of BaseLib interfaces to verify untrust= ed user data in UEFI Applications. The background of this problem is covered= in a bugzilla[1], and was also extensively discussed in the last proposed p= atch discussion thread[2]. We want the solution to the problem to land in th= e next stable tag: edk2-stable202005, and can submit the patch.

Currently there is no clear decision on what approach to ta= ke, as there are three different proposals to implement:

1) Andrew Fish=E2=80=99s approach with C11-like constraint handler= [3].
2) Marvin H=C3=A4user=E2=80=99s approach with return code= s and constraint assertions being moved to the caller side[4].
3) My approach suggesting DebugLib interface simplification, permitting us t= o resolve the problem and keep backwards compatibility[5].
My personal opinion is that:

=E2=80= =94 Marvin=E2=80=99s way is very interesting, but it would require implement= ing code on the caller side basically for every call, and this would require= s extra programming effort. It may also be uneasy to teach people how to do t= his right, and this does not strictly provide a good solution for situations= , where nesting is more than 2 (i.e. when user code calls library code, whic= h itself calls library code). Because of these downsides I am afraid it is i= mpractical to adopt it in EDK II.
=E2=80=94 Andrew=E2=80=99s w= ay is reasonable from the C standard point of view, but it does not work qui= te right with reentrancy and I am not positive we need the dynamic handling,= especially because of DEBUG/RELEASE build confusion. I explained this in my= previous e-mail, which I attached to the end of this message as quote, sinc= e groups.io <http://groups.io/> ate the previous send. Ot= her than that, I would say that I can implement this approach if there is no= other option.
=E2=80=94 My own approach makes sense, as it re= duces the amount of code in every DebugLib and actually simplifies the devel= opment in the future. I believe currently this is the most reasonable route w= e can take, and suggest other parties to consider it.

Since the three of us each have their own suggestions, I ask Mike and= others to rejoin our discussions and give their opinions quickly, so that w= e can choose, perhaps vote, and proceed with the implementation. Thanks for y= our dedication.

I support the orig= inal proposal given in:

https://bugzilla.ti= anocore.org/show_bug.cgi?id=3D2054#c0

also k= nown as the v3 posting:

* [edk2-devel] [PATCH v= 3 1/1]
MdePkg: Add PCD to disable safe string constraint asser= tions

https://edk2.groups.io/g/devel/message/52= 838
http://mid.mail-archive.com/20200103171242.63839-2-vit9696= @protonmail.com

With the following tweak: I thi= nk we should rather introduce a new bit
in "PcdDebugPropertyMa= sk" than an entirely new BOOLEAN PCD.


Laszlo,

+1
(Note that both PcdDebugPro= pertyMask and the propsed new PCD support
precisely the follow= ing PCD access methods: PcdsFixedAtBuild,
PcdsPatchableInModul= e.)

I don't feel too strongly about this part (= i.e., whether we introduce a
new PCD, or a new bit in PcdDebug= PropertyMask), as long as we make sure
that the access method c= an *never* become dynamic.

I do prefer quite st= rongly the original proposal, at a higher level.

Here's why I support the original proposal:

-= In a brand new code base (or brand new set of APIs), I would fight
tooth and nail to keep such ASSERT()s out of the *base* APIs. It's up t= o
the caller to check return values. Also, additional wrapper<= br class=3D"">functions/macros can be offered *on top* of the base APIs that=
internalize the ASSERT()s for special use cases.

But this ship has sailed, for edk2.


Crazy idea but we could add versions of AP= Is that don't do the checking as new APIs? That would make life easier for u= ntrusted code, Runtime Drivers, code running on the AP, and other places tha= t the ASSERTs could have side effects.=  

= I'm not sure I understand "new APIs" correctly.

If it's an entirely new set= of APIs, then I think it's not good for
Vitaly -- I understand Vitaly wants to use the existent codebase wit= hout
much churn, just with the ASS= ERTs removed.

If you mean a new library instance, that's different. If the p= ackage
owners do not mind the code= duplication that's inherent in creating such
an alternative library instance, then sure, it can work for me.


Laszlo,

Sorry the new APIs would be in addition, and the simplest way t= o cleanly solve your concern that I share about the caller not having contro= l. 

I don't want a long term goal t= o block getting the short term features needed by the community.
<= br class=3D"">
Thanks,

Andrew = Fish


- I'm u= nfriendly towards callbacks. They make the behavior of code
im= plicit rather than explicit, and implicit is more difficult to reason
about. IMO callbacks should be considered a last resort only.


Not my 1st choice either a= s I agree with your first point about given the caller control. I was just w= anted to go through the exercise of what it would take to make it like C11, a= nd give the caller control. 

Just my= two cents, because I've been asked to comment.
<= br class=3D"">Thanks for helping out.

Thanks!
L= aszlo

= --Apple-Mail-DFBB40DE-33CD-4AD8-82DF-11ABD6884DB4--