Re: [edk2-devel] Disabling safe string constraint assertions

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 6602 bytes --]

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 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:
> 
> 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’s approach with C11-like constraint handler[3].
> 2) Marvin Häuser’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:
> 
> — Marvin’s 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.
> — Andrew’s 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.

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 is not too problematic in a practical programming sense. Usually it is hardware drivers that have timer events, to manage things like DMA. The most common TPL usage is preventing reentrancy in the protocols, but in this case after a driver is loaded it is generally always called indirectly via its protocol APIs. It is common for applications to not run at elevated TPL at all. 

Thanks,

Andrew Fish

> — 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=2054 <https://bugzilla.tianocore.org/show_bug.cgi?id=2054>
> [2] https://edk2.groups.io/g/devel/topic/69401948 <https://edk2.groups.io/g/devel/topic/69401948>
> [3] https://edk2.groups.io/g/devel/message/54520 <https://edk2.groups.io/g/devel/message/54520>
> [4] https://edk2.groups.io/g/devel/message/54544 <https://edk2.groups.io/g/devel/message/54544>
> [5] https://edk2.groups.io/g/devel/message/54499 <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’s wait for Mike to comment before I consider starting the implementation. 
>> 
>> Best wishes,
>> Vitaly
>> 
>> [1] https://edk2.groups.io/g/devel/message/54499 <https://edk2.groups.io/g/devel/message/54499>
> 
> 


[-- Attachment #2: Type: text/html, Size: 10082 bytes --]