From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web09.15690.1583345937882080596 for ; Wed, 04 Mar 2020 10:18:58 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=BEIq4VfZ; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583345937; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=pPSrayx7A4Azkf+mpKXDjCEqmv4HzXMxA2zWUs2v/dU=; b=BEIq4VfZbE8C0NEiSBGv/68hCe8O+7pySM4VnqXdjbkx8g8FjNIn4Mm+RnnroeR8/8m7y+ TReMKiyl1hPYqd1zqPpbKpi7HrA8DC7PGETbXqt2xrhtA8QIkQfa16Dj7wFIJRaAz2tWEC wB3x08lk0hVu53fSuvIjyz5G1TJFq0c= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-187-MSWHyrm0OOyKNYejVF8mRw-1; Wed, 04 Mar 2020 13:18:50 -0500 X-MC-Unique: MSWHyrm0OOyKNYejVF8mRw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5F52518B9FC2; Wed, 4 Mar 2020 18:18:48 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-59.ams2.redhat.com [10.36.117.59]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6B57119C58; Wed, 4 Mar 2020 18:18:46 +0000 (UTC) Subject: Re: Disabling safe string constraint assertions To: Andrew Fish Cc: Vitaly Cheptsov , devel@edk2.groups.io, =?UTF-8?Q?Marvin_H=c3=a4user?= , Mike Kinney , "Gao, Liming" , "Gao, Zhichao" References: <17370CE3-AA53-4858-B497-1AB93DA709D3@ispras.ru> <86d328d2-e929-379a-e6dd-69968a064f13@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 4 Mar 2020 19:18:45 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 03/04/20 18:53, Andrew Fish wrote: >=20 >=20 >> On Mar 4, 2020, at 5:33 AM, Laszlo Ersek 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 m= y messages do not display in the web-interface and some of e-mail clients i= n 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 fu= ture. >>> >>> Back to the issue, we want to make constraint assertions (SAFE_STRING_C= ONSTRAINT_CHECK) in SafeString.c be optionally disabled in DEBUG builds, as= 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 covere= d 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 constrai= nt 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 requir= e 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 peop= le 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 li= brary 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 posi= tive we need the dynamic handling, especially because of DEBUG/RELEASE buil= d confusion. I explained this in my previous e-mail, which I attached to th= e end of this message as quote, since groups.io ate the= previous send. Other than that, I would say that I can implement this appr= oach 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 sugge= st other parties to consider it. >>> >>> 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= 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@protonmail.co= m >> >> 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.) >> >> 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. >> >=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= effects.=20 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. >=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 the= 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. Thanks! Laszlo