From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by mx.groups.io with SMTP id smtpd.web09.16271.1583348217943893488 for ; Wed, 04 Mar 2020 10:56:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=p3twR/XK; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.27/8.16.0.27) with SMTP id 024Iq0od064622; Wed, 4 Mar 2020 10:56:56 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=8qimBi/KqBLEc4GZVydLw5+Sp4zlwxbigNoiFa6gg6Q=; b=p3twR/XKg1qP5+7BThgYjAScsG55JT+bMF1vvV41J9+WD0WVUFIVRp5cd8I03vMaSvb0 EpKdzu6uuNZXRr2/jpvEp/a3fwm4N9g5V4ZTh/FmDb24slmwi8NYsGPi+sT6oFg5BVbq 0V4E2FJq2jcdeyumtJaSrvi//j1lwKXQDFKYN0KtPob3bPgBNe2nS6GmII8D1bFElOQN ++FUdCD6DEjoSA/zkhDxwSsm6vn7iJIiClhP0L9g2aDz7F7ohJG6b3bN1mts7r3cFBFi 2mFoTj7d1CvZ1cMe4fU8/Rixzq8W5zn3iZ7oanahPnnYFj/wNmpgcd0u91PG2p9GmuE6 fQ== Received: from rn-mailsvcp-mta-lapp04.rno.apple.com (rn-mailsvcp-mta-lapp04.rno.apple.com [10.225.203.152]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 2yfngwtgun-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 04 Mar 2020 10:56:56 -0800 Received: from nwk-mmpp-sz10.apple.com (nwk-mmpp-sz10.apple.com [17.128.115.122]) by rn-mailsvcp-mta-lapp04.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPS id <0Q6O006CHNAVV7D0@rn-mailsvcp-mta-lapp04.rno.apple.com>; Wed, 04 Mar 2020 10:56:55 -0800 (PST) Received: from process_milters-daemon.nwk-mmpp-sz10.apple.com by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0Q6O00D00MN9II00@nwk-mmpp-sz10.apple.com>; Wed, 04 Mar 2020 10:56:55 -0800 (PST) X-Va-A: X-Va-T-CD: 07a9f7dd315dc6000695a0402a47d12d X-Va-E-CD: 27402815c5e31821051163b02fab88e9 X-Va-R-CD: 8bf71fd521e6e2c0a106a042b8cb9e5a X-Va-CD: 0 X-Va-ID: b11c87dd-14b8-4ea7-8f98-c8579b7bce12 X-V-A: X-V-T-CD: 07a9f7dd315dc6000695a0402a47d12d X-V-E-CD: 27402815c5e31821051163b02fab88e9 X-V-R-CD: 8bf71fd521e6e2c0a106a042b8cb9e5a X-V-CD: 0 X-V-ID: ed81c1d5-7ffa-44b2-89c7-81e44df49ab9 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572 definitions=2020-03-04_08:2020-03-04,2020-03-04 signatures=0 Received: from [17.235.12.181] by nwk-mmpp-sz10.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0Q6O00EIRNATC550@nwk-mmpp-sz10.apple.com>; Wed, 04 Mar 2020 10:56:55 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: <5F8859C7-5776-4DF0-99F5-1D0BC56B50C4@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: Disabling safe string constraint assertions Date: Wed, 04 Mar 2020 10:56:53 -0800 In-reply-to: Cc: Vitaly Cheptsov , devel@edk2.groups.io, =?utf-8?Q?Marvin_H=C3=A4user?= , Mike Kinney , "Gao, Liming" , "Gao, Zhichao" To: Laszlo Ersek References: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> <86d328d2-e929-379a-e6dd-69968a064f13@redhat.com> X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572 definitions=2020-03-04_08:2020-03-04,2020-03-04 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_92AD87A9-D005-4553-BBF3-92B2E602BB87" --Apple-Mail=_92AD87A9-D005-4553-BBF3-92B2E602BB87 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > 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 my 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 nice 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 untrusted 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 patch discussion thread[2]. = We want the solution to the problem to land in the next stable tag: = edk2-stable202005, and can submit the patch. >>>>=20 >>>> Currently there is no clear decision on what approach to take, as = there 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 = constraint assertions being moved to the caller side[4]. >>>> 3) My approach suggesting DebugLib interface simplification, = permitting 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 = require implementing code on the caller side basically for every call, = and this would requires extra programming effort. It may also be uneasy = to teach people how to do this 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, which itself calls library code). Because = of these downsides I am afraid 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 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, since groups.io = ate the previous send. Other 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 reduces the amount of = code in every DebugLib and actually simplifies the development in the = future. I believe currently this is the most reasonable route we can = take, and suggest other parties to consider it. >>>>=20 >>>> 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 we can choose, perhaps vote, and proceed with the implementation. = Thanks for your 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.com >>>=20 >>> With the following tweak: I think we should rather introduce a new = bit >>> in "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD. >>>=20 >>=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 to >>> 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 >>=20 >> Crazy idea but we could add versions of APIs that don't do the = checking as 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 effects.=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 Laszlo, Sorry the new APIs would be in addition, and the simplest way to cleanly = solve your concern that I share about the caller not having control.=20 I don't want a long term goal to block getting the short term features = needed by the community. Thanks, Andrew Fish >>=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 >>=20 >> Not my 1st choice either as I agree with your first point about given = the caller control. I was just wanted to go through the exercise of what = it would 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=_92AD87A9-D005-4553-BBF3-92B2E602BB87 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

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

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


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

On 03/03/20 22:12, Vitaly Cheptsov wrote:
Hello,

I am creating a new thread for this issue, as for some reason = half of my 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 = <http://groups.io/>, = and it did not like to be quoted. It will be quite nice if all the = messages sent 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, as they break our ability to use some of = BaseLib interfaces to verify untrusted 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 patch discussion thread[2]. = We want the solution to the problem to land in the next stable tag: = edk2-stable202005, and can submit the patch.

Currently there is no clear decision on what approach to = take, 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=99= s approach with return codes and constraint assertions being moved to = the caller side[4].
3) My approach suggesting DebugLib = interface simplification, permitting us to 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 implementing code on the = caller side basically for every call, and this would requires extra = programming effort. It may also be uneasy to teach people how to do this = 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, which itself calls library code). Because of these = downsides I am afraid 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 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, since groups.io <http://groups.io/> ate the = previous send. Other 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 reduces the amount of code in every DebugLib = and actually simplifies the development in the future. I believe = currently this is the most reasonable route we 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 we can = choose, perhaps vote, and proceed with the implementation. Thanks for = your dedication.

I support the = original proposal given in:

https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054#c0
also known as the v3 posting:
* [edk2-devel] [PATCH v3 1/1]
MdePkg: Add PCD = to disable safe string constraint assertions

https://edk2.groups.io/g/devel/message/52838
http://mid.mail-archive.com/20200103171242.63839-2-vit9696@prot= onmail.com

With the following tweak: I = think we should rather introduce a new bit
in = "PcdDebugPropertyMask" than an entirely new BOOLEAN PCD.

Laszlo,

+1

(Note that both PcdDebugPropertyMask and the propsed new PCD = support
precisely the following PCD access methods: = PcdsFixedAtBuild,
PcdsPatchableInModule.)

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.

I = do prefer quite strongly 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 to
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.

But this ship has sailed, for edk2.


Crazy idea but we = could add versions of APIs that don't do the checking as 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 = 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 without
much churn, just with the ASSERTs removed.

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.


Laszlo,

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

I don't want a long = term goal to block getting the short term features needed by the = community.

Thanks,

Andrew Fish


- 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.


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

Just my two cents, = because I've been asked to comment.

Thanks for helping out.

Thanks!
Laszlo

= --Apple-Mail=_92AD87A9-D005-4553-BBF3-92B2E602BB87--