From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by mx.groups.io with SMTP id smtpd.web09.1701.1583274752927396368 for ; Tue, 03 Mar 2020 14:32:33 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=pnpJQOU5; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id 023MWBBX002633; Tue, 3 Mar 2020 14:32:31 -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=RhYyejHEmiPBvFzjOj3FmBpbowdYN7WSsdQPw338Zn4=; b=pnpJQOU5kZGrS3ZhGqM1yf+8Y+pk0whbelsfxed7YHoltXAU8BIy5xDv9nkWAfU/2ymg QysiFqDDp/IRSOM1MI0zuQjVmTEYos3vF00BoVG6ugYxFBhW6+p2V6d942vIifruiCGH X4SH4RmyTEVpYOlHVtblXms96divj4Ruyas6M0X/rJN5k9RiqQpm2YDxk76SXBi3g9HS tvYOiKWC4gkW+3iA4GXciveQ29bL3uEX75sA+/nGRe3ygp0MDchJV76rl9QpGDUp/US2 M3FwKM+RuSTGZBtk19LxLFMHP2rvLXgyAI2PXyTTIklho0qNNmMVKywq33xkxUiieUV7 jA== Received: from rn-mailsvcp-mta-lapp01.rno.apple.com (rn-mailsvcp-mta-lapp01.rno.apple.com [10.225.203.149]) by nwk-aaemail-lapp03.apple.com with ESMTP id 2yg9365dev-5 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Tue, 03 Mar 2020 14:32:31 -0800 Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by rn-mailsvcp-mta-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.1.20190704 64bit (built Jul 4 2019)) with ESMTPS id <0Q6N0104L2M6OZJ0@rn-mailsvcp-mta-lapp01.rno.apple.com>; Tue, 03 Mar 2020 14:32:30 -0800 (PST) Received: from process_milters-daemon.nwk-mmpp-sz12.apple.com by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0Q6N00C001Q2XU00@nwk-mmpp-sz12.apple.com>; Tue, 03 Mar 2020 14:32:30 -0800 (PST) X-Va-A: X-Va-T-CD: 2c323b19c9bf43c24bf8f3f2bb9cbf14 X-Va-E-CD: 5fe8742df1f24b2a63953342b8aec60f X-Va-R-CD: a28cdce158fb57d7593fd497a2f3bfd9 X-Va-CD: 0 X-Va-ID: 5b5fb762-2f8f-4a7d-bbe5-04a790ddfb43 X-V-A: X-V-T-CD: 2c323b19c9bf43c24bf8f3f2bb9cbf14 X-V-E-CD: 5fe8742df1f24b2a63953342b8aec60f X-V-R-CD: a28cdce158fb57d7593fd497a2f3bfd9 X-V-CD: 0 X-V-ID: d766cf72-ae0a-49f1-a391-9054670ef330 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138,18.0.572 definitions=2020-03-03_07:2020-03-03,2020-03-03 signatures=0 Received: from [17.235.12.181] by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0Q6N0070V2M4CL60@nwk-mmpp-sz12.apple.com>; Tue, 03 Mar 2020 14:32:30 -0800 (PST) Sender: afish@apple.com From: "Andrew Fish" Message-id: <81624D89-A775-40EB-AB63-83AB6CE2C070@apple.com> MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] Disabling safe string constraint assertions Date: Tue, 03 Mar 2020 14:32:28 -0800 In-reply-to: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> Cc: =?utf-8?Q?Marvin_H=C3=A4user?= , Mike Kinney , "Gao, Liming" , "Gao, Zhichao" , Laszlo Ersek To: devel@edk2.groups.io, cheptsov@ispras.ru References: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> 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-03_07:2020-03-03,2020-03-03 signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_D51C536F-549E-482F-B590-3A4B65BE5267" --Apple-Mail=_D51C536F-549E-482F-B590-3A4B65BE5267 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Vitaly, I was planing on bringing it up at the Stewards meeting that was scheduled= for today, but it got rescheduled to next week. So if we don't get tractio= n on the mailing list I'll bring it up in the meeting.=20 > On Mar 3, 2020, at 1:12 PM, Vitaly Cheptsov wrote: >=20 > 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.i= o , and it did not like to be quoted. It will be quite n= ice if all the messages sent are plain-text to avoid such issues in the fut= ure. >=20 > Back to the issue, we want to make constraint assertions (SAFE_STRING_CO= NSTRAINT_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= 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 constrain= t 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 wo= uld 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 = for situations, where nesting is more than 2 (i.e. when user code calls lib= rary code, which itself calls library code). Because of these downsides I a= m 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 o= f view, but it does not work quite right with reentrancy and I am not posit= ive 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 appro= ach if there is no other option. I agree that a save restore would be required to deal with TPL issues, so = my initial API set was incomplete and I added the extra API.=20 Given every EFI driver and application is bound to its own static library = instance managing TPL is not too problematic in a practical programming sen= se. Usually it is hardware drivers that have timer events, to manage things= like DMA. The most common TPL usage is preventing reentrancy in the protoc= ols, but in this case after a driver is loaded it is generally always calle= d indirectly via its protocol APIs. It is common for applications to not ru= n at elevated TPL at all.=20 Thanks, Andrew Fish > =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 sugges= t other parties to consider it. >=20 > Since the three of us each have their own suggestions, I ask Mike and ot= hers to rejoin our discussions and give their opinions quickly, so that we = can choose, perhaps vote, and proceed with the implementation. Thanks for y= our dedication. >=20 > Best wishes, > Vitaly >=20 > [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 >=20 >=20 >> 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 SafeStrin= gLib is that it indeed tried to follow Microsoft's initial design (later co= ntributed as a C standard Annex K) but completely ignored the ability to co= nfigure the handling logic. This is the problem we actually try to resolve = here. >>=20 >> The reality behind this annex is, however, that very few implementation= s 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 th= eir uses it is not strictly necessary or is not easily achievable. For exam= ple, 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 lik= ely use of the constraint handler configuration will be not changing it bef= ore 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 wh= ether the need to have it configurable in runtime is actually needed. Or ra= ther 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. Ano= ther issue of runtime configuration is binary size, but this I believe shou= ld 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. Howeve= r, 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 i= mplementation.=20 >>=20 >> Best wishes, >> Vitaly >>=20 >> [1] https://edk2.groups.io/g/devel/message/54499 >=20 >=20 --Apple-Mail=_D51C536F-549E-482F-B590-3A4B65BE5267 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Vitaly,
I was planing on bringing it up at the S= tewards meeting that was scheduled for today, but it got rescheduled to nex= t week. So if we don't get traction on the mailing list I'll bring it up in= the meeting. 

On Mar 3, 2020, at 1:12 PM, Vitaly Cheptsov= <cheptsov@ispras.ru> wrote:

<= meta http-equiv=3D"Content-Type" content=3D"text/html; charset=3Dutf-8" cla= ss=3D"">
Hello,

I am cre= ating a new thread for this issue, as for some reason half of my messages d= o not display in the web-interface and some of e-mail clients in the previo= us one. It seems that somebody sent broken HTML code to groups.io, and it did not like to be quoted. It w= ill be quite nice if all the messages sent are plain-text to avoid such iss= ues in the future.

Back to the issue, we want to make constraint assertions (SAFE_STRING_CON= STRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as t= hey 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 p= atch discussion thread[2]. We want the solution to the problem to land in t= he 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 an= d constraint assertions being moved to the caller side[4].
3) My approach suggesting DebugLib interface simplification, permitt= ing us to resolve the problem and keep backwards compatibility[5].

My personal opinion is th= at:

=E2=80=94 Mar= vin=E2=80=99s way is very interesting, but it would require implementing co= de on the caller side basically for every call, and this would requires ext= ra 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 i= mpractical to adopt it in EDK II.
=E2=80=94 Andrew=E2=80=99s way is reasonab= le 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&nbs= p;groups.io ate the previ= ous send. Other than that, I would say that I can implement this approach i= f there is no other option.
=
I agree that a save restore would be required to = deal with TPL issues, so my initial API set was incomplete and I added the = extra API. 

Given every EFI driver= and application is bound to its own static library instance managing TPL i= s not too problematic in a practical programming sense. Usually it is hardw= are drivers that have timer events, to manage things like DMA. The most com= mon TPL usage is preventing reentrancy in the protocols, but in this case a= fter a driver is loaded it is generally always called indirectly via its pr= otocol APIs. It is common for applications to not run at elevated TPL at al= l. 

Thanks,

Andrew Fish

=E2=80=94 My own approach makes sense, as it reduces the amount of code i= n every DebugLib and actually simplifies the development in the future. I b= elieve 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 M= ike and others to rejoin our discussions and give their opinions quickly, s= o that we can choose, perhaps vote, and proceed with the implementation. Th= anks 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] <= a href=3D"https://edk2.groups.io/g/devel/message/54520" class=3D"">https://= edk2.groups.io/g/devel/message/54520
[4]&nb= sp;http= s://edk2.groups.io/g/devel/message/54544
[5] = https:/= /edk2.groups.io/g/devel/message/54499


Andrew,<= /span>

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 functionali= ty from Bounds Checking Annex from C11 is an interesting idea. The historic= al part of our SafeStringLib is that it indeed tried to= follow Microsoft's initial design (later contributed as a C stan= dard 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 implem= entations agreed to adopt it, including C standard library in macOS. The de= sign of its functions permits some level of flexibility, but I think for mo= st of their uses it is not strictly necessary or is not easily achieva= ble. For example, one of the largest problems of this annex is that it enti= rely 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 execut= ion.

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 th= e constraint handler configuration will be not changing it before the call = to a function, but rather configuring close to the entry point of an a= pplication or a driver. For this exact reason it is questionable whether th= e 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, whethe= r he will able to do it properly without shooting into the leg. Another iss= ue of runtime configuration is binary size, but this I believe should be ea= sy 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=_D51C536F-549E-482F-B590-3A4B65BE5267--