From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a01:111:f400:fe45::715; helo=nam02-cy1-obe.outbound.protection.outlook.com; envelope-from=bret.barkelew@microsoft.com; receiver=edk2-devel@lists.01.org Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0715.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe45::715]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 760E421CF1D04 for ; Tue, 13 Feb 2018 08:50:22 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=kYe5uSuh4t4HvMUqODnMzoLU0LiksGoyGSldTDmZv88=; b=k6ozyczxy5QYNc7mUK0v9hy8hFCT52RRpVY7YoJ5K+ZLKdjOFHLD/7ZDp8sGgoOkCUpPW8jqyWUaZyjr1cmHe4FX6T5AnXsbi8X+JtEatGCO2ZShqtK04AUkDLaqjgEXuGUiIafWuc0JxTRaeSyQzC+rXOlM15Mjr74XrCSeLMM= Received: from DM5PR2101MB0936.namprd21.prod.outlook.com (52.132.131.166) by DM5PR2101MB1110.namprd21.prod.outlook.com (52.132.131.167) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.527.2; Tue, 13 Feb 2018 16:56:10 +0000 Received: from DM5PR2101MB0936.namprd21.prod.outlook.com ([fe80::9486:dd86:a56d:57a5]) by DM5PR2101MB0936.namprd21.prod.outlook.com ([fe80::9486:dd86:a56d:57a5%3]) with mapi id 15.20.0527.000; Tue, 13 Feb 2018 16:56:08 +0000 From: Bret Barkelew To: "Kinney, Michael D" , Laszlo Ersek , Sean Brogan CC: "edk2-devel@lists.01.org" , "Gao, Liming" Thread-Topic: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance Thread-Index: AQHTeQC0wxWzfgg2NUe7h+R6mF9YuaOZ9oYAgAADoYCACJ7MgIAAQWsAgAAKtcQ= Date: Tue, 13 Feb 2018 16:56:07 +0000 Message-ID: References: <20171219193625.16060-1-michael.d.kinney@intel.com> <656eb64b-3265-f021-ff4f-df2ed6b7c752@redhat.com> <868954f3-f368-073c-9e62-d11440e719c9@redhat.com>, In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [40.112.210.181] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DM5PR2101MB1110; 7:xyDLEvZR7402CPMnqBLWF84MSJUS2G2QEGHNbMGbsU7dYNSPN2RcRBRfoQax84BYeA0jD60UXvwtWhFhma/V9agzNKt3fBFoWQmYRXrOLjQQTAwZ7dAcziqg7CmHgLtyBk4x+LORsWdRMKi00vQup8XAlCJ2DWiVbm8pdjN9b84HNYU1jCO1WkdG7KTLpBnuleWb+437wNxXBP4dYLOcgkuA5dXqfo8tljqFwGLb7Z3xScaU17BSKEo6yMvK6nFU x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-forefront-antispam-report: SFV:SKI; SCL:-1; SFV:NSPM; SFS:(10019020)(346002)(366004)(376002)(396003)(39860400002)(39380400002)(60444003)(13464003)(189003)(199004)(22452003)(2906002)(316002)(81156014)(6436002)(8676002)(5660300001)(54906003)(186003)(110136005)(54896002)(584604001)(606006)(74316002)(93886005)(6636002)(33656002)(68736007)(9686003)(1511001)(236005)(55016002)(3660700001)(6306002)(3280700002)(3846002)(6116002)(81166006)(66066001)(53936002)(8936002)(8990500004)(6346003)(72206003)(102836004)(59450400001)(6246003)(14454004)(76176011)(86362001)(478600001)(99286004)(2950100002)(10290500003)(25786009)(26005)(10090500001)(5250100002)(2900100001)(97736004)(106356001)(7696005)(53546011)(966005)(6506007)(86612001)(105586002)(7736002)(229853002)(4326008); DIR:OUT; SFP:1102; SCL:1; SRVR:DM5PR2101MB1110; H:DM5PR2101MB0936.namprd21.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 7ecd72cf-e152-4f7c-3de7-08d57302ad7e x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(4604075)(3008032)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603307)(7193020); SRVR:DM5PR2101MB1110; x-ms-traffictypediagnostic: DM5PR2101MB1110: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(28532068793085)(158342451672863)(89211679590171)(189930954265078)(788757137089)(162533806227266)(81439100147899)(219752817060721)(228905959029699)(17755550239193); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(61425038)(6040501)(2401047)(5005006)(8121501046)(3002001)(3231101)(944501161)(10201501046)(93006095)(93001095)(6055026)(61426038)(61427038)(6041288)(20161123560045)(20161123558120)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(6072148)(201708071742011); SRVR:DM5PR2101MB1110; BCL:0; PCL:0; RULEID:; SRVR:DM5PR2101MB1110; x-forefront-prvs: 0582641F53 received-spf: None (protection.outlook.com: microsoft.com does not designate permitted sender hosts) authentication-results: spf=none (sender IP is ) smtp.mailfrom=Bret.Barkelew@microsoft.com; x-microsoft-antispam-message-info: 7/y1YyVP92FURyApahFfkDpFA4j2vwO5ppNhqNU2qUECDcJF0lGl6oWQZy4VEB5Qd0DIWFeSgtgAWiWnVezBNw== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7ecd72cf-e152-4f7c-3de7-08d57302ad7e X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Feb 2018 16:56:07.8942 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR2101MB1110 X-Content-Filtered-By: Mailman/MimeDel 2.1.23 Subject: Re: [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 13 Feb 2018 16:50:23 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable In response to the original question, I would content that our goal should = be "a". We should be allowing universal detection of errors without the cal= ler having to carry this detection code itself. The analog would be the safe string functions: if a buffer overflow occurs,= they don't find a way to "fix" the operation, but they faithfully report a= n error. As such, I believe from my review that these functions work as intended. - Bret ________________________________ From: Kinney, Michael D Sent: Tuesday, February 13, 2018 8:17:48 AM To: Laszlo Ersek; Sean Brogan; Bret Barkelew Cc: edk2-devel@lists.01.org; Gao, Liming Subject: RE: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and= instance +Bret Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] > On Behalf Of Laszlo Ersek > Sent: Tuesday, February 13, 2018 4:24 AM > To: Kinney, Michael D ; Sean > Brogan > Cc: edk2-devel@lists.01.org; Gao, Liming > > Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add > SafeIntLib class and instance > > Sean, Michael, > > can you please follow up on this? > > To clarify, I think this is a serious bug in SafeIntLib, > dependent on > what we want to use this library for. As far as I > understand, SafeIntLib > intends to centralize integer manipulation / arithmetic, > so that client > code need not concern itself with overflow checking and > such (on the C > expression level -- it still has to check return > statuses, of course). > In other words, undefined behavior related to integer > arithmetic is > supposed to be prevented in various modules by moving all > such > operations into SafeIntLib, and letting client code use > SafeIntLib APIs. > > However, for this to work, SafeIntLib itself must be 100% > free of > undefined behavior. And that's not the case (unless we > define additional > guarantees -- on top of ISO C -- for edk2 target > architectures). Should > I file a TianoCore BZ? Or is someone already (re)auditing > the library? > Or else, is my concern unjustified? Please comment. > > Thanks, > Laszlo > > On 02/08/18 01:45, Laszlo Ersek wrote: > > On 02/08/18 01:32, Laszlo Ersek wrote: > >> On 12/19/17 20:36, Kinney, Michael D wrote: > >>> From: Sean Brogan > >>> > >>> SafeIntLib provides helper functions to prevent > integer overflow > >>> during type conversion, addition, subtraction, and > multiplication. > >> > >> I clearly cannot review such a huge patch, but I've > noticed something > >> and would like to ask for clarification: > >> > >>> +/** > >>> + INT64 Subtraction > >>> + > >>> + Performs the requested operation using the input > parameters into a value > >>> + specified by Result type and stores the converted > value into the caller > >>> + allocated output buffer specified by Result. The > caller must pass in a > >>> + Result buffer that is at least as large as the > Result type. > >>> + > >>> + If Result is NULL, RETURN_INVALID_PARAMETER is > returned. > >>> + > >>> + If the requested operation results in an overflow > or an underflow condition, > >>> + then Result is set to INT64_ERROR and > RETURN_BUFFER_TOO_SMALL is returned. > >>> + > >>> + @param[in] Minuend A number from which > another is to be subtracted. > >>> + @param[in] Subtrahend A number to be subtracted > from another > >>> + @param[out] Result Pointer to the result of > subtraction > >>> + > >>> + @retval RETURN_SUCCESS Successful > subtraction > >>> + @retval RETURN_BUFFER_TOO_SMALL Underflow > >>> + @retval RETURN_INVALID_PARAMETER Result is NULL > >>> +**/ > >>> +RETURN_STATUS > >>> +EFIAPI > >>> +SafeInt64Sub ( > >>> + IN INT64 Minuend, > >>> + IN INT64 Subtrahend, > >>> + OUT INT64 *Result > >>> + ) > >>> +{ > >>> + RETURN_STATUS Status; > >>> + INT64 SignedResult; > >>> + > >>> + if (Result =3D=3D NULL) { > >>> + return RETURN_INVALID_PARAMETER; > >>> + } > >>> + > >>> + SignedResult =3D Minuend - Subtrahend; > >>> + > >>> + // > >>> + // Subtracting a positive number from a positive > number never overflows. > >>> + // Subtracting a negative number from a negative > number never overflows. > >>> + // If you subtract a negative number from a > positive number, you expect a positive result. > >>> + // If you subtract a positive number from a > negative number, you expect a negative result. > >>> + // Overflow if inputs vary in sign and the output > does not have the same sign as the first input. > >>> + // > >>> + if (((Minuend < 0) !=3D (Subtrahend < 0)) && > >>> + ((Minuend < 0) !=3D (SignedResult < 0))) { > >>> + *Result =3D INT64_ERROR; > >>> + Status =3D RETURN_BUFFER_TOO_SMALL; > >>> + } else { > >>> + *Result =3D SignedResult; > >>> + Status =3D RETURN_SUCCESS; > >>> + } > >>> + > >>> + return Status; > >>> +} > >> > >> Is our goal to > >> > >> (a) catch overflows before the caller goes wrong due > to them, or > >> (b) completely prevent undefined behavior from > happening, even inside > >> SafeInt64Sub()? > >> > >> The above implementation may be good for (a), but it's > not correct for > >> (b). The > >> > >> SignedResult =3D Minuend - Subtrahend; > >> > >> subtraction invokes undefined behavior if the result > cannot be > >> represented [*]; the rest of the code cannot help. > >> > >> Now if we say that such subtractions always occur > according to the > >> "usual two's complement definition", on all > architectures that edk2 > >> targets, and we're sure that no compiler or analysis > tool will flag -- > >> or exploit -- the UB either, then the code is fine; > meaning our choice > >> is (a). > >> > >> But, if (b) is our goal, I would replace the current > error condition with: > >> > >> (((Subtrahend > 0) && (Minuend < MIN_INT64 + > Subtrahend)) || > >> ((Subtrahend < 0) && (Minuend > MAX_INT64 + > Subtrahend))) > > > > To clarify, I wouldn't just replace the error > condition. In addition to > > that, I would remove the SignedResult helper variable > (together with the > > current subtraction), and calculate & assign > > > > *Result =3D Minuend - Subtrahend; > > > > only after the error condition fails (i.e. the > subtraction is safe). > > > > Thanks, > > Laszlo > > > > > >> Justification: > >> > >> * Subtrahend=3D=3D0 can never cause overflow > >> > >> * Subtrahend>0 can only cause overflow at the negative > end, so check > >> that: (Minuend - Subtrahend < MIN_INT64), > mathematically speaking. > >> In order to write that down in C, add Subtrahend (a > positive value) > >> to both sides, yielding (Minuend < MIN_INT64 + > Subtrahend). Here, > >> (MIN_INT64 + Subtrahend) will never go out of range, > because > >> Subtrahend is positive, and (MIN_INT64 + MAX_INT64) > is representable. > >> > >> * Subtrahend<0 can only cause overflow at the positive > end, so check > >> that: (Minuend - Subtrahend > MAX_INT64), > mathematically speaking. > >> In order to write that down in C, add Subtrahend (a > negative value) > >> to both sides, yielding (Minuend > MAX_INT64 + > Subtrahend). Here, > >> (MAX_INT64 + Subtrahend) will never go out of range, > because > >> Subtrahend is negative, and (MAX_INT64 + MIN_INT64) > is representable. > >> > >> ( > >> > >> [*] ISO C99 section 6.5 Expressions, p5: "If an > exceptional condition > >> occurs during the evaluation of an expression (that > is, if the result is > >> not mathematically defined or not in the range of > representable values > >> for its type), the behavior is undefined." > >> > >> Section 6.2.5 Types, p9 only exempts unsigned > integers, "A computation > >> involving unsigned operands can never overflow, > because a result that > >> cannot be represented by the resulting unsigned > integer type is reduced > >> modulo the number that is one greater than the largest > value that can be > >> represented by the resulting type." > >> > >> Note that this is different from conversion, where the > computation first > >> succeeds (=3D we have a value), and then the value is > converted to a type > >> that causes truncation: section 6.3.1.3 Signed and > unsigned integers, > >> p3: "Otherwise, the new type is signed and the value > cannot be > >> represented in it; either the result is > implementation-defined or an > >> implementation-defined signal is raised." > >> > >> In the code above, the expression (Minuend - > Subtrahend) can invoke > >> undefined behavior, there is no conversion (not even > as part of the > >> assignment to SignedResult). > >> > >> ) > >> > >> Thanks, > >> Laszlo > >> _______________________________________________ > >> edk2-devel mailing list > >> edk2-devel@lists.01.org > >> https://na01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flis= ts.01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=3D04%7C01%7CBret.Barkelew%= 40microsoft.com%7C87f15d7947fe45fee17a08d572fd542d%7C72f988bf86f141af91ab2d= 7cd011db47%7C1%7C0%7C636541354724483642%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4= wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=3DrFfax%2BkpyD= xZt9UPmIT9tdBFC1KOeq3Xhfudm00XcC0%3D&reserved=3D0 > >> > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://na01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flist= s.01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=3D04%7C01%7CBret.Barkelew%4= 0microsoft.com%7C87f15d7947fe45fee17a08d572fd542d%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C636541354724483642%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4w= LjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=3DrFfax%2BkpyDx= Zt9UPmIT9tdBFC1KOeq3Xhfudm00XcC0%3D&reserved=3D0 > > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://na01.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Flists.= 01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=3D04%7C01%7CBret.Barkelew%40m= icrosoft.com%7C87f15d7947fe45fee17a08d572fd542d%7C72f988bf86f141af91ab2d7cd= 011db47%7C1%7C0%7C636541354724483642%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj= AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=3DrFfax%2BkpyDxZt= 9UPmIT9tdBFC1KOeq3Xhfudm00XcC0%3D&reserved=3D0