From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=104.47.34.93; helo=nam01-by2-obe.outbound.protection.outlook.com; envelope-from=bret.barkelew@microsoft.com; receiver=edk2-devel@lists.01.org Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0093.outbound.protection.outlook.com [104.47.34.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 7B20F21CF1D0B for ; Tue, 13 Feb 2018 10:13:24 -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=J6jgNM06btBjxVp9z1iuraohsxN/fSRloJrjzi6t0aw=; b=hREPLxJJblg6fB4IcHQCtXh05Jf4UDPHxJtn7FgP5McEetKxxs3LTlN8JKpKImwWoJsF5ie6FwleT0KZ/kt8XDX8fSspQiWqReSHt7viBM8QET5a9Ad+9znCqYcXq90d1Os6BGUIbu5GUssw351637JupNFGeqxVJfnXinZJW/M= Received: from DM5PR2101MB0936.namprd21.prod.outlook.com (52.132.131.166) by DM5PR2101MB0728.namprd21.prod.outlook.com (10.167.110.153) 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 18:19:13 +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 18:19:12 +0000 From: Bret Barkelew To: Laszlo Ersek , "Kinney, Michael D" , Sean Brogan CC: "edk2-devel@lists.01.org" , "Gao, Liming" Thread-Topic: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and instance Thread-Index: AQHTeQC0wxWzfgg2NUe7h+R6mF9YuaOZ9oYAgAADoYCACJ7MgIAAQWsAgAAKtcSAAAlmgIAADbNr Date: Tue, 13 Feb 2018 18:19:12 +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> , <753b0fa9-cd72-ae78-d810-4ee39f4535a5@redhat.com> In-Reply-To: <753b0fa9-cd72-ae78-d810-4ee39f4535a5@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [131.107.32.41] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DM5PR2101MB0728; 7:okvAWlXoEc6baQ7YtxXnCrUfuZLxhvj7WdBrAyWq2oFnfYzNLiNYrAvIYDxYbhhdb9nAPeaFapxPrwwyn5VmXdOWd6Zg+1HKQI4X+nmhMHWx4mLcYlf0sGrFzMA98fYAGpYhSEVS2MUmqcY1Tz923xmwZE4YwlKCOPb03QGnsgoHwdq0Wkpxs69JTfinVHJbxgfPq0lRVadN8jLl6dp23jptImAGj4m5RPwGTennPTC2VTiE+HcJZTZnl4b8C19j x-ms-exchange-antispam-srfa-diagnostics: SSOS;SSOR; x-o365ent-eop-header: Message processed by - O365_ENT: Allow from ranges (Engineering ONLY) x-forefront-antispam-report: SFV:SKI; SCL:-1; SFV:NSPM; SFS:(10019020)(39380400002)(39860400002)(396003)(366004)(346002)(376002)(60444003)(199004)(189003)(97736004)(3280700002)(55016002)(229853002)(86362001)(3660700001)(25786009)(59450400001)(8936002)(53546011)(6506007)(54896002)(236005)(66066001)(6346003)(4326008)(106356001)(6306002)(9686003)(1511001)(86612001)(6116002)(3846002)(93886005)(53936002)(7696005)(2900100001)(6636002)(10090500001)(2950100002)(186003)(6436002)(76176011)(10290500003)(105586002)(8676002)(81156014)(8990500004)(81166006)(68736007)(99286004)(478600001)(110136005)(54906003)(5660300001)(7736002)(2906002)(26005)(6246003)(316002)(33656002)(22452003)(5250100002)(74316002)(72206003)(14454004)(102836004); DIR:OUT; SFP:1102; SCL:1; SRVR:DM5PR2101MB0728; H:DM5PR2101MB0936.namprd21.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 283dea38-aa53-453f-0ef8-08d5730e4883 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(48565401081)(5600026)(4604075)(3008032)(4534165)(4627221)(201703031133081)(201702281549075)(2017052603307)(7193020); SRVR:DM5PR2101MB0728; x-ms-traffictypediagnostic: DM5PR2101MB0728: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(28532068793085)(162533806227266)(21748063052155)(228905959029699); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(61425038)(6040501)(2401047)(5005006)(8121501046)(3002001)(93006095)(93001095)(3231101)(944501161)(10201501046)(6055026)(61426038)(61427038)(6041288)(20161123560045)(20161123558120)(20161123564045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(6072148)(201708071742011); SRVR:DM5PR2101MB0728; BCL:0; PCL:0; RULEID:; SRVR:DM5PR2101MB0728; 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: YpkNnk3RSC/1sfi3eFmaXdGdm7kxa19G8zOVEs/e6/rMMjJIrvSyq6AASvL1543+D9tYXki7PKoXJ3YXHVjiZQ== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: 283dea38-aa53-453f-0ef8-08d5730e4883 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Feb 2018 18:19:12.4196 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR2101MB0728 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 18:13:25 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Ah, yes, this makes more sense. It was difficult to review all your comment= s on my phone. Thanks for clarifying! - Bret From: Laszlo Ersek Sent: Tuesday, February 13, 2018 9:29 AM To: Bret Barkelew; Kinney, Michael D; Sean Brogan Cc: edk2-devel@lists.01.org; Gao, Liming Subject: Re: [edk2] [Patch] MdePkg/BaseSafeIntLib: Add SafeIntLib class and= instance On 02/13/18 17:56, Bret Barkelew wrote: > 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 caller having to carry this detection code itself. OK. The question is how the detection is implemented internally. Is that implementation (in edk2) allowed to rely on behavior that the ISO C standard leaves undefined, and -- consequently -- compilers might exploit for code generation? > 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 an error. Precisely. Correspondingly, translated to the safe string functions, my question becomes: Is the implementation of the safe string functions allowed to employ buffer overflow internally, and detect the overflow after the fact? > As such, I believe from my review that these functions work as > intended. Please let me quote the function again, from "MdePkg/Library/BaseSafeIntLib/SafeIntLib.c": 3831 RETURN_STATUS 3832 EFIAPI 3833 SafeInt64Sub ( 3834 IN INT64 Minuend, 3835 IN INT64 Subtrahend, 3836 OUT INT64 *Result 3837 ) 3838 { 3839 RETURN_STATUS Status; 3840 INT64 SignedResult; 3841 3842 if (Result =3D=3D NULL) { 3843 return RETURN_INVALID_PARAMETER; 3844 } 3845 3846 SignedResult =3D Minuend - Subtrahend; 3847 3848 // 3849 // Subtracting a positive number from a positive number never ove= rflows. 3850 // Subtracting a negative number from a negative number never ove= rflows. 3851 // If you subtract a negative number from a positive number, you = expect a positive result. 3852 // If you subtract a positive number from a negative number, you = expect a negative result. 3853 // Overflow if inputs vary in sign and the output does not have t= he same sign as the first input. 3854 // 3855 if (((Minuend < 0) !=3D (Subtrahend < 0)) && 3856 ((Minuend < 0) !=3D (SignedResult < 0))) { 3857 *Result =3D INT64_ERROR; 3858 Status =3D RETURN_BUFFER_TOO_SMALL; 3859 } else { 3860 *Result =3D SignedResult; 3861 Status =3D RETURN_SUCCESS; 3862 } 3863 3864 return Status; 3865 } On line 3846, integer overflow is possible. If that happens, not only is the resultant value of "SignedResult" undefined, but the behavior of the rest of the function is undefined, according to ISO C. In other words, just because we move the checking into a library function, we cannot check *after the fact*. The subtraction on line 3846 can invoke undefined behavior *first*, and we check only afterwards, starting on line 3855. Here's an analogy. Various C compilers regularly equate "undefined behavior" with "never happens" (which is a valid interpretation of the ISO C standard for the compiler to make). For example, given the code int f(int *x) { *x =3D 3; if (x =3D=3D NULL) { return 1; } return 0; } the compiler may compile f() to unconditionally return 0, such as: int f(int *x) { *x =3D 3; return 0; } Because, the (x =3D=3D NULL) branch would depend on undefined behavior invoked by the assignment to *x. Given that the (*x =3D 3) assignment is undefined for (x=3D=3DNULL), according to ISO C, the subsequent (x =3D=3D N= ULL) check can be taken as constant false, and eliminated. Similarly, a sufficiently smart compiler may assume that the subtraction on line 3846 will never overflow. (Because, such an overflow would be undefined behavior.) Consequently, it may deduce that the overflow checks, *after the fact*, evaluate to constant false, and can be eliminated. It might compile the function as in: { RETURN_STATUS Status; INT64 SignedResult; if (Result =3D=3D NULL) { return RETURN_INVALID_PARAMETER; } SignedResult =3D Minuend - Subtrahend; *Result =3D SignedResult; Status =3D RETURN_SUCCESS; return Status; } I apoligize if I'm unclear; I really don't know how to put it better. The subtraction on line 3846 runs the risk of undefined behavior, and the checks starting on line 3855 are too late. Thanks Laszlo