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.web09.614.1583269949163763654 for ; Tue, 03 Mar 2020 13:12:30 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: ispras.ru, ip: 83.149.199.45, mailfrom: cheptsov@ispras.ru) Received: from [127.0.0.1] (unknown [77.232.9.83]) by mail.ispras.ru (Postfix) with ESMTPSA id DD81A64F; Wed, 4 Mar 2020 00:12:26 +0300 (MSK) From: Vitaly Cheptsov Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\)) Subject: Disabling safe string constraint assertions Message-Id: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> Date: Wed, 4 Mar 2020 00:12:25 +0300 Cc: "Gao, Liming" , "Gao, Zhichao" , Laszlo Ersek To: devel@edk2.groups.io, =?utf-8?Q?Marvin_H=C3=A4user?= , Mike Kinney , Andrew Fish X-Mailer: Apple Mail (2.3608.60.0.2.5) X-Groupsio-MsgNum: 55346 Content-Type: multipart/signed; boundary="Apple-Mail=_D674AC2B-3045-4F11-A3C9-0C1CADF61257"; protocol="application/pgp-signature"; micalg=pgp-sha512 --Apple-Mail=_D674AC2B-3045-4F11-A3C9-0C1CADF61257 Content-Type: multipart/alternative; boundary="Apple-Mail=_28F58C9A-A490-4472-8DD1-42F03B8FA745" --Apple-Mail=_28F58C9A-A490-4472-8DD1-42F03B8FA745 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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 , 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=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]. 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 = 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. Best wishes, Vitaly [1] https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054 = [2] https://edk2.groups.io/g/devel/topic/69401948 = [3] https://edk2.groups.io/g/devel/message/54520 = [4] https://edk2.groups.io/g/devel/message/54544 = [5] https://edk2.groups.io/g/devel/message/54499 = > Andrew, >=20 > I am not sure you received my message with the explanations[1], where = I believe I covered the nuances, but it is nice to see you on the tune. >=20 > Making EDK II replicate more of the functionality from Bounds Checking = Annex from C11 is an interesting idea. The historical part of our = SafeStringLib is that it indeed tried to follow Microsoft's initial = design (later contributed as a C standard Annex K) but completely = ignored the ability to configure the handling logic. This is the problem = we actually try to resolve here. >=20 > The reality behind this annex is, however, that very few = implementations agreed to adopt it, including C standard library in = macOS. The design of its functions permits some level of flexibility, = but I think for most of their uses it is not strictly necessary or is = not easily achievable. For example, one of the largest problems of this = annex is that it entirely ignores threads or reentrancy. In EDK II we do = not (yet) have threads in their true sense, but we do have events, which = can interrupt code execution. >=20 > The code you presented is not strictly safe, as without TPL adjustment = it may affect the behaviour of some random event handler. Thus the most = likely use of the constraint handler configuration will be not changing = it before the call to a function, but rather configuring close to the = entry point of an application or a driver. For this exact reason it is = questionable whether the need to have it configurable in runtime is = actually needed. Or rather it is questionable whether anybody will = actually use it and if he does, whether he will able to do it properly = without shooting into the leg. Another issue of runtime configuration is = binary size, but this I believe should be easy to avoid within macros = body through pcds that disable assertions before calling the active = handler. >=20 > All in all, I believe the suggested approach is also fine to us. = However, I would personally suggest my approach, as I believe it is = cleaner. For now let=E2=80=99s wait for Mike to comment before I = consider starting the implementation. >=20 > Best wishes, > Vitaly >=20 > [1] https://edk2.groups.io/g/devel/message/54499 = --Apple-Mail=_28F58C9A-A490-4472-8DD1-42F03B8FA745 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
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, 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=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].

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

Best wishes,
Vitaly

[1] https://bugzilla.tianocore.org/show_bug.cgi?id=3D2054
=
[2] https://edk2.groups.io/g/devel/topic/69401948
[3] https://edk2.groups.io/g/devel/message/54520
[4] https://edk2.groups.io/g/devel/message/54544
[5] https://edk2.groups.io/g/devel/message/54499


Andrew,

I am not sure you received my = message with the explanations[1], where I believe I covered the nuances, = but it is nice to see you on the tune.

Making EDK II replicate more of the functionality from = Bounds Checking Annex from C11 is an interesting idea. The historical = part of our SafeStringLib is that it indeed tried to = follow Microsoft's initial design (later contributed as a C = standard Annex K) but completely ignored the ability to = configure the handling logic. This is the problem we actually try to = resolve here.

The reality behind this annex is, however, = that very few implementations agreed to adopt it, including C standard = library in macOS. The design of its functions permits some level of = flexibility, but I think for most of their uses it is not strictly = necessary or is not easily achievable. For example, one of the = largest problems of this annex is that it entirely ignores threads or = reentrancy. In EDK II we do not (yet) have threads in their true sense, = but we do have events, which can interrupt code execution.

The code you presented is not strictly safe, as = without TPL adjustment it may affect the behaviour of some random event = handler. Thus the most likely use of the constraint handler = configuration will be not changing it before the call to a function, but = rather configuring close to the entry point of an application or a = driver. For this exact reason it is questionable whether the need to = have it configurable in runtime is actually needed. Or rather it is = questionable whether anybody will actually use it and if he does, = whether he will able to do it properly without shooting into the leg. = Another issue of runtime configuration is binary size, but this I = believe should be easy to avoid within macros body through pcds that = disable assertions before calling the active handler.

All in all, I believe the suggested approach is = also fine to us. However, I would personally suggest my approach, as I = believe it is cleaner. For now let=E2=80=99s wait for Mike to comment = before I consider starting the implementation. 

Best wishes,
Vitaly

[1] https://edk2.groups.io/g/devel/message/54499


= = --Apple-Mail=_28F58C9A-A490-4472-8DD1-42F03B8FA745-- --Apple-Mail=_D674AC2B-3045-4F11-A3C9-0C1CADF61257 Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename=signature.asc Content-Type: application/pgp-signature; name=signature.asc Content-Description: Message signed with OpenPGP -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEsLABAI5Y5VbvBdmpL8K2O86Eyz4FAl5eyDkACgkQL8K2O86E yz7SUA//cRXC+TjBjTlxEpEivoRSOg26247db0WQVOR+lkp1dIsds5Ip6bSSOlfa L7ASUgvzs0p6oq6wYC3O5/YQiCw2rglxQtwdBByA45NU6miPL9n6aLrizwys2cu3 ZS3K+JxU9LjZ3jvOsYdWokqCGEwWchBBxPG0Xjfn7T74KKvCj/uwieYfA4GfmTDB xXxGuDyF5lzQuNFuzToMlz85inVsMURY47HNhr7CyHx8a88hqcrJBV/kS+ZsZNmj jZAnKUA+2HPISoavH3vq6CatoZhwkm8HOJ8XTVcoy+XK63IvTEX+gkqxwniEWuPl z8GaGkUcmnf7mFcj3I4/Ualm3Ziv+7qa/BAoXpzMCcqYmc21ISuu6Ac6MqCZ87HO LsHCGla5oDJNh8ZEYj9HsCtyRgh+Vm/Bchso23LOypHqkq+YBVQOl+FU2y6NN3LN UA6LLh7qlfRAil0KfEDMv/KXqUyuK/wXNWFCBVgIHOUkJeWeArhA8suQ1ndnEVEw +w4YaJ119GIF2Yh5I/L+scK8AXq/uD3muGlJlHrTBBc8uRPdn4E894slRxci+OGy J76Jwc5xvfgTnHGvfZLiLLH9mjQRBzIkSFI8neO/HVWIB3C0PIDOz6fOIPZxCI/n u5YKbsE2i94P3zxwCNm1cqogiTKaC5R29uTclvzPsa+sEBPbn3w= =VRgq -----END PGP SIGNATURE----- --Apple-Mail=_D674AC2B-3045-4F11-A3C9-0C1CADF61257--