From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from a7-10.smtp-out.eu-west-1.amazonses.com (a7-10.smtp-out.eu-west-1.amazonses.com [54.240.7.10]) by mx.groups.io with SMTP id smtpd.web10.7228.1676382464919551047 for ; Tue, 14 Feb 2023 05:47:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ipxe.org header.s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2 header.b=ikpIQStu; spf=pass (domain: eu-west-1.amazonses.com, ip: 54.240.7.10, mailfrom: 01020186502e636e-e8e67b6b-f07a-43b9-aa39-35be69d08c68-000000@eu-west-1.amazonses.com) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=cphpx6z2rfcgehlykjjh3gknqe3hsoe2; d=ipxe.org; t=1676382463; h=Message-ID:Date:MIME-Version:To:Cc:References:From:Subject:In-Reply-To:Content-Type:Content-Transfer-Encoding; bh=0oV4FFzqakcbrdahScuVYkM1hjfGZLgmZXhSVh0MDb4=; b=ikpIQStulYVYonpHRLdYr5GDhP4CaN0zi0HNwlvE0z3Hn/YTmPuSLzMXFmV9rRp9 6uwlG6NN3wvsV7LaV6A6FNe5Tfx/Z8/LAcnKWAyRbZsvNqWrijEhmvv5rSNMQCH3X4Z NffqY61E1MVUVnGDyD5hbVAbchKEqvAn/OQWjFJUFCqDVgvTelCDs6amF7A7NTg95tJ k7z284diD8nGeLCs6oVUDR83bMv1DUI0MGJCBfsp2LPccpsA8AHbbTyCo02xF+sAH/s yuD83/u8wXHIfR1HgeCr95UIOJbFOHu0yhotk2Ga+PsLZb6e0a9xMOEYcJ1JbdDoQR1 S5y48qGxeA== DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/simple; s=ihchhvubuqgjsxyuhssfvqohv7z3u4hn; d=amazonses.com; t=1676382463; h=Message-ID:Date:MIME-Version:To:Cc:References:From:Subject:In-Reply-To:Content-Type:Content-Transfer-Encoding:Feedback-ID; bh=0oV4FFzqakcbrdahScuVYkM1hjfGZLgmZXhSVh0MDb4=; b=jBQ0T2ashrK3envDReWf9Klp2xYqQblm9LvFnC6B/LQ2iHpDfG2tFtPdSGFNJ9NC sYCQxorlDRMmxprJajUfPnTBaafhlIwNjdh3MGRn8J2VZ954Dt5mIq/5KSQh8E/adWx gk7cyiXtj0LsI9hz5TeWHycbqHMQeSY6Cg+otmFU= Message-ID: <01020186502e636e-e8e67b6b-f07a-43b9-aa39-35be69d08c68-000000@eu-west-1.amazonses.com> Date: Tue, 14 Feb 2023 13:47:42 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 To: devel@edk2.groups.io, kraxel@redhat.com Cc: mikuback@linux.microsoft.com, Dandan Bi , Erich McMillan , Jian J Wang , Liming Gao , Star Zeng , Zhichao Gao , Zhiguang Liu , Michael Kubacki References: <20230213154908.1993-1-mikuback@linux.microsoft.com> <20230213154908.1993-2-mikuback@linux.microsoft.com> <010201864b8f56cb-c9b052f6-c9e6-4c22-9d99-c87c947a7169-000000@eu-west-1.amazonses.com> <20230214130114.kp4z4zmfjgaalv47@sirius.home.kraxel.org> From: "Michael Brown" Subject: Re: [edk2-devel] [PATCH v3 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts In-Reply-To: <20230214130114.kp4z4zmfjgaalv47@sirius.home.kraxel.org> X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on blyat.fensystems.co.uk Feedback-ID: 1.eu-west-1.fspj4M/5bzJ9NLRzJP0PaxRwxrpZqiDQJ1IF94CF2TA=:AmazonSES X-SES-Outgoing: 2023.02.14-54.240.7.10 Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 14/02/2023 13:01, Gerd Hoffmann wrote: > On Mon, Feb 13, 2023 at 04:15:30PM +0000, Michael Brown wrote: >> On 13/02/2023 15:48, Michael Kubacki wrote: >>> @@ -1608,9 +1610,12 @@ ParseAndAddExistingSmbiosTable ( >>> // >>> // Make sure not to access memory beyond SmbiosEnd >>> // >>> - if ((Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw) || >>> - (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw)) >>> - { >>> + Status = SafeUintnAdd ((UINTN)Smbios.Raw, sizeof (SMBIOS_STRUCTURE), &SafeIntResult); >>> + if (EFI_ERROR (Status)) { >>> + return EFI_INVALID_PARAMETER; >>> + } >>> + >>> + if ((SafeIntResult > (UINTN)SmbiosEnd.Raw) || (SafeIntResult < (UINTN)Smbios.Raw)) { >>> return EFI_INVALID_PARAMETER; >>> } >> >> Could this not be expressed much more cleanly as just: >> >> if ((UINTN)(SmbiosEnd.Raw - Smbios.Raw) < sizeof (SMBIOS_STRUCTURE)) { >> return EFI_INVALID_PARAMETER; >> } >> >> without the need to hide a basic arithmetic operation behind an ugly wrapper >> function and drag in an external library? > > Well, the advantage of using SafeIntLib is that (a) it is obvious even > to untrained code readers what is going on here, and (b) it is hard to > get it wrong. > > When looking at the quality some of the patches being posted to the list > have I'd say that is important to consider even if you personally have > no problems avoiding integer overflows without the help of SafeIntLib. Fair enough. But in that case it should be used in a way that makes it clear what it's actually doing. Specifically, the check for if (... || (SafeIntResult < (UINTN)Smbios.Raw)) { ... } then becomes meaningless, since the whole point of SafeUintnAdd() is that it cannot result in this kind of unsigned integer wraparound. The code as modified by this patch is *harder* to understand, because the reader has to dig through to figure out why this check still exists, look at the implementation of SafeUintnAdd() to see what is meant by "safe" in this context, and finally come to the conclusion that the remaining underflow check is redundant code that should have been removed. The reader is also going to be confused by the fact that the code as modified by this patch then contains two separate methods for checking for integer overflows, since the patch does not similarly update the very similar code found almost immediately afterwards: if ((Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) > SmbiosEnd.Raw) || (Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) < Smbios.Raw)) { return EFI_INVALID_PARAMETER; } Lastly: the actual operation being made safe here is "check that buffer contains at least this much data remaining". This is most obviously done by calculating the remaining buffer space (i.e. (UINTN)(SmbiosEnd.Raw - Smbios.Raw)) and comparing against that. That logic is clear, simple, and obviously correct at first glance. Using the SafeUintnAdd() call does something that *is* mathematically equivalent to this check, but where the logic is much harder to parse. I'd prefer it if the code were updated to avoid SafeUintnAdd() altogether. But if not, then at a minimum the redundant check should be removed, and the calculation involving Smbios.Hdr->Length should also be updated to use SafeUintnAdd(). Thanks, Michael