From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=17.151.62.25; helo=mail-in2.apple.com; envelope-from=afish@apple.com; receiver=edk2-devel@lists.01.org Received: from mail-in2.apple.com (mail-out2.apple.com [17.151.62.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2D5A421CF1D01 for ; Tue, 13 Feb 2018 09:09:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1518542126; x=2382455726; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=OgC0YShMu4RIPtfW4e3eNIHoJcv+qrwQFo/JCzxeYy4=; b=pJAHKKB3WqsR0yMDWw610WZ7oIcpbZwDFMvZAb9rek+7u0PKwe9JxBog3xFAckgo WufBrp2qBO96diVLHw0bhcE9b8KD95dbOFRUsy34vIz4rrGOuzzN7VbzLx83YIJz CsJxYBoO0ytYySazSC4jy1VnOu5/HKrEMF1Tw3Vjf25XndJEkElRTF0JM/sxN4i6 54RtFTqAhXdX/ExmrlbFYeiT2fH76jFEQgrth2hBd3kTeuBzDOXOBQyFrp4Szxxi zfqE7X4t3YkjZ/hhV+fAZB72pIxF8ns0JgSOK3OcYUUtfGVOpgP4HzUhR3A9H9oV jZrc8yg4ltajOdueTgVqjg==; Received: from relay4.apple.com (relay4.apple.com [17.128.113.87]) (using TLS with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mail-in2.apple.com (Apple Secure Mail Relay) with SMTP id 05.CF.13823.E2D138A5; Tue, 13 Feb 2018 09:15:26 -0800 (PST) X-AuditID: 11973e11-d4eb59e0000035ff-70-5a831d2ef8d5 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by relay4.apple.com (Apple SCV relay) with SMTP id 52.24.11536.E2D138A5; Tue, 13 Feb 2018 09:15:26 -0800 (PST) MIME-version: 1.0 Received: from [17.235.60.212] by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.2.20180130 64bit (built Jan 30 2018)) with ESMTPSA id <0P4300J54MLO6R40@nwk-mmpp-sz13.apple.com>; Tue, 13 Feb 2018 09:15:26 -0800 (PST) Sender: afish@apple.com From: Andrew Fish Message-id: <20838843-EB3C-4EA1-8D05-65AF847FB642@apple.com> Date: Tue, 13 Feb 2018 09:15:23 -0800 In-reply-to: Cc: Mike Kinney , Laszlo Ersek , Sean Brogan , "edk2-devel@lists.01.org" , "Gao, Liming" To: Bret Barkelew References: <20171219193625.16060-1-michael.d.kinney@intel.com> <656eb64b-3265-f021-ff4f-df2ed6b7c752@redhat.com> <868954f3-f368-073c-9e62-d11440e719c9@redhat.com> X-Mailer: Apple Mail (2.3445.5.20) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrGLMWRmVeSWpSXmKPExsUi2FAYrqsn2xxlsOGthsW7LQvZLPYcOsps sezYDhaLFfc2sFt0dPxjslj4aDOzA5vH4j0vmTy6Z/9j8Wjd8Zfd4/2+q2wBLFFcNimpOZll qUX6dglcGY+vtbEWfPrDXLHneRtLA+P2Q8xdjJwcEgImElOf9gLZXBxCAquZJP6+Ws8Ok3jf vZoNInGIUWJL7zdGkASvgKDEj8n3WEBsZoEwiaeP+tghir4ySrzaeRysW1hAXOLdmU1gK9gE lCVWzP/ADtFsI7Hu1hGomjCJtW8OgNWwCKhKtO36BmZzCiRLXO34wgIylFngBqPEkQXTWUES IgL6Eg+657BAbPvDJLFz7haoJ5Qkpn+/DXarhMAJNokD9/8wTmAUmoXk3FlIzoWwtSS+P2oF inMA2fISB8/LQoQ1JZ7d+wRVoi3x5N0F1gWMbKsYhXITM3N0M/OM9BILCnJS9ZLzczcxgiJq up3gDsbjq6wOMQpwMCrx8G541RQlxJpYVlyZe4hRmoNFSZx3//PGKCGB9MSS1OzU1ILUovii 0pzU4kOMTBycUg2MT56eX3D/3FWZ4rUMNkwm6QJT0ho+Odxs0XiyQWH1RKGfhYUfH+RzXw1f Y9d59SdvMqfL5lc8BxgO277Zev5EPGO7aM+0b4zpVTunM02Ivsj4o3VJaOd9S34jD5evnzjF rkofWta61OS6m7zRoZgdWxVv/NxvvM/18DsTP9OgC88lUy97cB85o8RSnJFoqMVcVJwIAAzF IH2JAgAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrJLMWRmVeSWpSXmKPExsUi2FB8Q1dPtjnK4P5nJYt3WxayWew5dJTZ YtmxHSwWK+5tYLfo6PjHZLHw0WZmBzaPxXteMnl0z/7H4tG64y+7x/t9V9kCWKK4bFJSczLL Uov07RK4Mh5fa2Mt+PSHuWLP8zaWBsbth5i7GDk5JARMJN53r2brYuTiEBI4xCixpfcbI0iC V0BQ4sfkeywgNrNAmMTTR33sEEVfGSVe7TzODpIQFhCXeHdmE9gkNgFliRXzP7BDNNtIrLt1 BKomTGLtmwNgNSwCqhJtu76B2ZwCyRJXO76wgAxlFrjBKHFkwXRWkISIgL7Eg+45LBDb/jBJ 7Jy7BepWJYnp32+zTWDkn4XkwllILoSwtSS+P2oFinMA2fISB8/LQoQ1JZ7d+wRVoi3x5N0F 1gWMbKsYBYpScxIrTfQSCwpyUvWS83M3MYIjoDB8B+O/ZVaHGAU4GJV4eDe8aooSYk0sK67M BQYTB7OSCG/CfaAQb0piZVVqUX58UWlOavEhRmkOFiVx3tu9QCmB9MSS1OzU1ILUIpgsEwen VAPj49fz0yq/HjsT3LPp3fK7ryv8lx5ZdHbVhtMJnwI/6nTKJXBNEpON4NjpMKEmp+zE053X TAQSP9U+36C64wfPjnvuL1O1v7Gf1HCK1he7xXSv7q4Js7Bg2a8dr24d5YxW/WC8nFlGfKP6 Uz6uNrk+K+Vr3+zNK/Z/4Ny1Z8L92OyJk6ZN2ytmr8RSnJFoqMVcVJwIAAxg5Tl8AgAA 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 17:09:36 -0000 Content-Type: text/plain; CHARSET=US-ASCII Content-Transfer-Encoding: 7BIT > On Feb 13, 2018, at 8:56 AM, 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. > > 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. > > As such, I believe from my review that these functions work as intended. > Bret, I think Lazlo's point is that undefined behavior[1] can cause the math function to break in the future and that we have to be very pedantic in how it is coded. Per the C standard it is legal for the compiler to optimized away undefined behavior[2], and clang is very aggressive about warning on undefined behavior and then updating the optimizer to remove the code in a future release. For example the BaseTool compression code broke with Xcode 9 recently due to the presence of an illegal 32-bit shift that was only hit when the optimizer inlined the function. While the compiler tries to emit warnings, or at least traps, for undefined behavior what we have seen with the Link Time Optimization is the code can just get removed. [1] - Kind of clangs point of view on undefined behavior in C: http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html [2] - Example of undefined behavior in clang that emits a trap. Dereferencing NULL is undefined behavior in C so clang emits a trap, and optimizes way the code after the trap. ~/work/Compiler>cat undefined.c int main () { int *Yikes = 0; *Yikes = 1; return 0; } ~/work/Compiler>clang -S -Os undefined.c ~/work/Compiler>cat undefined.S .section __TEXT,__text,regular,pure_instructions .macosx_version_min 10, 12 .globl _main _main: ## @main .cfi_startproc ## BB#0: pushq %rbp Lcfi0: .cfi_def_cfa_offset 16 Lcfi1: .cfi_offset %rbp, -16 movq %rsp, %rbp Lcfi2: .cfi_def_cfa_register %rbp ud2 .cfi_endproc .subsections_via_symbols Thanks, Andrew Fish > - 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 == NULL) { >>>>> + return RETURN_INVALID_PARAMETER; >>>>> + } >>>>> + >>>>> + SignedResult = 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) != (Subtrahend < 0)) && >>>>> + ((Minuend < 0) != (SignedResult < 0))) { >>>>> + *Result = INT64_ERROR; >>>>> + Status = RETURN_BUFFER_TOO_SMALL; >>>>> + } else { >>>>> + *Result = SignedResult; >>>>> + Status = 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 = 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 = Minuend - Subtrahend; >>> >>> only after the error condition fails (i.e. the >> subtraction is safe). >>> >>> Thanks, >>> Laszlo >>> >>> >>>> Justification: >>>> >>>> * Subtrahend==0 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 (= 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=https%3A%2F%2Flists.01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C87f15d7947fe45fee17a08d572fd542d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636541354724483642%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=rFfax%2BkpyDxZt9UPmIT9tdBFC1KOeq3Xhfudm00XcC0%3D&reserved=0 >>>> >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C87f15d7947fe45fee17a08d572fd542d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636541354724483642%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=rFfax%2BkpyDxZt9UPmIT9tdBFC1KOeq3Xhfudm00XcC0%3D&reserved=0 >>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.01.org%2Fmailman%2Flistinfo%2Fedk2-devel&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C87f15d7947fe45fee17a08d572fd542d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636541354724483642%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwifQ%3D%3D%7C-1&sdata=rFfax%2BkpyDxZt9UPmIT9tdBFC1KOeq3Xhfudm00XcC0%3D&reserved=0 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel