From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.7356.1676382787385399536 for ; Tue, 14 Feb 2023 05:53:07 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=hEi7rMqP; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id 85ECB20E56A4; Tue, 14 Feb 2023 05:53:05 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 85ECB20E56A4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1676382786; bh=J27CpEdKXazFpbA6VZ7pxA9gDZBFB5vUvAj4oEFY1GU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=hEi7rMqPuV6HP4pCMRBxG3GwuNilr+zBdgjQGRe8w7c/3i7mJbkQbPniBvmnw6pl4 1PYnTIHOJu1ZMPsEUjrVg2hcvIrf/f+BrSoboAn4aXK4LS+HkUyyu0Fv/sMBdYIkxq 1rYw1FbS6fwG+fxuBchgxJX/BFl73qCXQk5wUJ1I= Message-ID: <920ec08a-e7c7-06c3-de2e-b54dd12f5db9@linux.microsoft.com> Date: Tue, 14 Feb 2023 08:53:04 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [edk2-devel] [PATCH v3 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts To: Gerd Hoffmann , devel@edk2.groups.io, mcb30@ipxe.org Cc: 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 Kubacki" In-Reply-To: <20230214130114.kp4z4zmfjgaalv47@sirius.home.kraxel.org> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Either approach works for me. I understand the desire to avoid code bloat that comes with the library. The most common classes of issues I see at the moment are asserts being misused for error handling (which is significant), general issues with integer conversion/evaluation, and unsafe arithmetic operations. I suppose this is in the spirit of Gerd's comment as I thought additional library usage might increase awareness to help with the latter. Do the MdeModulePkg / SMBIOS maintainers have a preference? Thanks, Michael On 2/14/2023 8:01 AM, 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. > > take care, > Gerd