[edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions

Vitaly Cheptsov via Groups.Io vit9696=protonmail.com at groups.io
Fri Feb 14 17:37:55 UTC 2020


Michael,

Generalising the approach makes good sense to me, but we need to make an obvious distinguishable difference between:
- precondition and invariant assertions (i.e. assertions, where function will NOT work if they are violated)
- constraint asserts (i.e. assertions, which allow us to spot unintentional behaviour when parsing untrusted data, but which do not break function behaviour).

As we want to use this outside of SafeString,  I suggest the following:
- Introduce DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED 0x40 bit for PcdDebugPropertyMask instead of PcdAssertOnSafeStringConstraints.
- Introduce DebugAssertConstraintEnabled DebugLib function to check for DEBUG_PROPERTY_ASSERT_CONSTRAINT_ENABLED.
- Introduce ASSERT_CONSTRAINT macro, that will assert only if DebugConstraintAssertEnabled returns true.
- Change SafeString ASSERTS in SAFE_STRING_CONSTRAINT_CHECK to ASSERT_CONSTRAINTs.
- Use ASSERT_CONSTRAINT in other places where necessary.

I believe this way lines best with EDK II design. If there are no objections, I can submit the patch in the beginning of next week.

Best wishes,
Vitaly

> 14 февр. 2020 г., в 20:00, Kinney, Michael D <michael.d.kinney at intel.com> написал(а):
> 
> Vitaly,
>
> I want to make sure a feature PCD can be used to disable ASSERT() behavior in more than just safe string functions in BaseLib.
>
> Can we consider changing the name and description of PcdAssertOnSafeStringConstraints to be more generic, so if we find other lib APIs, the name will make sense?
>
> Maybe something like: PcdEnableLibraryAssertChecks?  Default is TRUE.  Can set to FALSE in DSC file to disable ASSERT() checks.
>
> Thanks,
>
> Mike
>
>
>
> From: devel at edk2.groups.io <mailto:devel at edk2.groups.io> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> On Behalf Of Vitaly Cheptsov via Groups.Io
> Sent: Friday, February 14, 2020 3:55 AM
> To: Kinney, Michael D <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>; Gao, Liming <liming.gao at intel.com <mailto:liming.gao at intel.com>>; Gao, Zhichao <zhichao.gao at intel.com <mailto:zhichao.gao at intel.com>>; devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> Cc: Marvin Häuser <marvin.haeuser at outlook.com <mailto:marvin.haeuser at outlook.com>>; Laszlo Ersek <lersek at redhat.com <mailto:lersek at redhat.com>>
> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to disable safe string constraint assertions
>
> Replying as per Liming's request for this to be merged into edk2-stable202002.
>
> On Mon, Feb 10, 2020 at 14:12, vit9696 <vit9696 at protonmail.com <mailto:vit9696 at protonmail.com>> wrote:
> Hello,
> 
> It has been quite some time since we submitted the patch with so far no negative response. As I mentioned previously, my team will strongly benefit from its landing in EDK II mainline. Since it does not add any regressions and can be viewed as a feature implementation for the rest of EDK II users, I request this to be merged upstream in edk2-stable202002.
> 
> Best wishes,
> Vitaly
> 
> > 27 янв. 2020 г., в 12:47, vit9696 <vit9696 at protonmail.com <mailto:vit9696 at protonmail.com>> написал(а):
> > 
> > 
> > Hi Mike,
> > 
> > Any progress with this? We would really benefit from this landing in the next stable release.
> > 
> > Best,
> > Vitaly
> > 
> >> 8 янв. 2020 г., в 19:35, Kinney, Michael D <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>> написал(а):
> >> 
> >> 
> >> Hi Vitaly,
> >> 
> >> Thanks for the additional background. I would like
> >> a couple extra day to review the PCD name and the places
> >> the PCD might potentially be used.
> >> 
> >> If we find other APIs where ASSERT() behavior is only
> >> valuable during dev/debug to quickly identify misuse
> >> with trusted data and the API provides predicable
> >> return behavior when ASSERT() is disabled, then I would
> >> like to have a pattern we can potentially apply to all
> >> these APIs across all packages.
> >> 
> >> Thanks,
> >> 
> >> Mike
> >> 
> >>> -----Original Message-----
> >>> From: devel at edk2.groups.io <mailto:devel at edk2.groups.io> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> On
> >>> Behalf Of Vitaly Cheptsov via Groups.Io
> >>> Sent: Monday, January 6, 2020 10:44 AM
> >>> To: Kinney, Michael D <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>>
> >>> Cc: devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> >>> Subject: Re: [edk2-devel] [PATCH v3 0/1] Add PCD to
> >>> disable safe string constraint assertions
> >>> 
> >>> Hi Mike,
> >>> 
> >>> Yes, the primary use case is for UEFI Applications. We
> >>> do not want to disable ASSERT’s completely, as
> >>> assertions that make sense, i.e. the ones signalising
> >>> about interface misuse, are helpful for debugging.
> >>> 
> >>> I have already explained in the BZ that basically all
> >>> safe string constraint assertions make no sense for
> >>> handling untrusted data. We find this use case very
> >>> logical, as these functions behave properly with
> >>> assertions disabled and cover all these error
> >>> conditions by the return statuses. In such situation is
> >>> not useful for these functions to assert, as we end up
> >>> inefficiently reimplementing the logic. I would have
> >>> liked the approach of discussing the interfaces
> >>> individually, but I struggle to find any that makes
> >>> sense from this point of view.
> >>> 
> >>> AsciiStrToGuid will ASSERT when the length of the
> >>> passed string is odd. Functions that cannot, ahem,
> >>> parse, for us are pretty much useless.
> >>> AsciiStrCatS will ASSERT when the appended string does
> >>> not fit the buffer. For us this logic makes this
> >>> function pretty much equivalent to deprecated and thus
> >>> unavailable AsciiStrCat, except it is also slower.
> >>> 
> >>> My original suggestion was to remove the assertions
> >>> entirely, but several people here said that they use
> >>> them to verify usage errors when handling trusted data.
> >>> This makes good sense to me, so we suggest to support
> >>> both cases by introducing a PCD in this patch.
> >>> 
> >>> Best wishes,
> >>> Vitaly
> >>> 
> >>>> 6 янв. 2020 г., в 21:28, Kinney, Michael D
> >>> <michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>> написал(а):
> >>>> 
> >>>> 
> >>>> Hi Vitaly,
> >>>> 
> >>>> Is the use case for UEFI Applications?
> >>>> 
> >>>> There is a different mechanism to disable all
> >>> ASSERT()
> >>>> statements within a UEFI Application.
> >>>> 
> >>>> If a component is consuming data from an untrusted
> >>> source,
> >>>> then that component is required to verify the
> >>> untrusted
> >>>> data before passing it to a function that clearly
> >>> documents
> >>>> is input requirements. If this approach is followed,
> >>> then
> >>>> the BaseLib functions can be used "as is" as long as
> >>> the
> >>>> ASSERT() conditions are verified before calling.
> >>>> 
> >>>> If there are some APIs that currently document their
> >>> ASSERT()
> >>>> behavior and we think that ASSERT() behavior is
> >>> incorrect and
> >>>> should be handled by an existing error return value,
> >>> then we
> >>>> should discuss each of those APIs individually.
> >>>> 
> >>>> Mike
> >>>> 
> >>>> 
> >>>>> -----Original Message-----
> >>>>> From: devel at edk2.groups.io <mailto:devel at edk2.groups.io> <devel at edk2.groups.io <mailto:devel at edk2.groups.io>> On
> >>>>> Behalf Of Vitaly Cheptsov via Groups.Io
> >>>>> Sent: Friday, January 3, 2020 9:13 AM
> >>>>> To: devel at edk2.groups.io <mailto:devel at edk2.groups.io>
> >>>>> Subject: [edk2-devel] [PATCH v3 0/1] Add PCD to
> >>> disable
> >>>>> safe string constraint assertions
> >>>>> 
> >>>>> REF:
> >>>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2054 <https://bugzilla.tianocore.org/show_bug.cgi?id=2054>
> >>>>> 
> >>>>> Requesting for merge in edk2-stable202002.
> >>>>> 
> >>>>> Changes since V1:
> >>>>> - Enable assertions by default to preserve the
> >>> original
> >>>>> behaviour
> >>>>> - Fix bugzilla reference link
> >>>>> - Update documentation in BaseLib.h
> >>>>> 
> >>>>> Vitaly Cheptsov (1):
> >>>>> MdePkg: Add PCD to disable safe string constraint
> >>>>> assertions
> >>>>> 
> >>>>> MdePkg/MdePkg.dec | 6 ++
> >>>>> MdePkg/Library/BaseLib/BaseLib.inf | 11 +--
> >>>>> MdePkg/Include/Library/BaseLib.h | 74
> >>>>> +++++++++++++-------
> >>>>> MdePkg/Library/BaseLib/SafeString.c | 4 +-
> >>>>> MdePkg/MdePkg.uni | 6 ++
> >>>>> 5 files changed, 71 insertions(+), 30 deletions(-)
> >>>>> 
> >>>>> --
> >>>>> 2.21.0 (Apple Git-122.2)
> >>>>> 
> >>>>> 
> >>>>> -=-=-=-=-=-=
> >>>>> Groups.io <http://groups.io/> Links: You receive all messages sent to
> >>> this
> >>>>> group.
> >>>>> 
> >>>>> View/Reply Online (#52837):
> >>>>> https://edk2.groups.io/g/devel/message/52837 <https://edk2.groups.io/g/devel/message/52837>
> >>>>> Mute This Topic:
> >>> https://groups.io/mt/69401948/1643496 <https://groups.io/mt/69401948/1643496>
> >>>>> Group Owner: devel+owner at edk2.groups.io <mailto:devel+owner at edk2.groups.io>
> >>>>> Unsubscribe: https://edk2.groups.io/g/devel/unsub <https://edk2.groups.io/g/devel/unsub>
> >>>>> [michael.d.kinney at intel.com <mailto:michael.d.kinney at intel.com>]
> >>>>> -=-=-=-=-=-=
> >>>> 
> >>> 
> >>> 
> >>> 
> >> 
> > 
> 
>
>
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#54461): https://edk2.groups.io/g/devel/message/54461
Mute This Topic: https://groups.io/mt/69401948/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200214/081b8e8c/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200214/081b8e8c/attachment.sig>


More information about the edk2-devel-archive mailing list