From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124])
 by mx.groups.io with SMTP id smtpd.web10.6095.1676379684202215546
 for <devel@edk2.groups.io>;
 Tue, 14 Feb 2023 05:01:24 -0800
Authentication-Results: mx.groups.io;
 dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UkfTPA7B;
 spf=pass (domain: redhat.com, ip: 170.10.129.124, mailfrom: kraxel@redhat.com)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
	s=mimecast20190719; t=1676379683;
	h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
	 to:to:cc:cc:mime-version:mime-version:content-type:content-type:
	 in-reply-to:in-reply-to:references:references;
	bh=ekVN9wur2PDEbV6h0U22bAjwkUjHa07lb7fqOISuUGo=;
	b=UkfTPA7BlVMX1yxI4n8E/zvSDLqz+v2MBwJsGDmaswxenrvWiLjUdG54/dBW+RwsH5wuMf
	L/XYG7eJYHHW1ZmjqgKTGXu/+1GRJbqvgC15W5pxU8rXZPfzzSs/xwbZIYKiVHaLzZTxS6
	cWaE15caNFF2pnaGgqBfpdbDVHZhUlI=
Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com
 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 us-mta-53-GmUJPxSZPLW3d8bS4IZYJQ-1; Tue, 14 Feb 2023 08:01:17 -0500
X-MC-Unique: GmUJPxSZPLW3d8bS4IZYJQ-1
Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 0DF9529ABA07;
	Tue, 14 Feb 2023 13:01:17 +0000 (UTC)
Received: from sirius.home.kraxel.org (ovpn-195-41.brq.redhat.com [10.40.195.41])
	by smtp.corp.redhat.com (Postfix) with ESMTPS id 885B81121318;
	Tue, 14 Feb 2023 13:01:15 +0000 (UTC)
Received: by sirius.home.kraxel.org (Postfix, from userid 1000)
	id 33C7A1802382; Tue, 14 Feb 2023 14:01:14 +0100 (CET)
Date: Tue, 14 Feb 2023 14:01:14 +0100
From: "Gerd Hoffmann" <kraxel@redhat.com>
To: devel@edk2.groups.io, mcb30@ipxe.org
Cc: mikuback@linux.microsoft.com, Dandan Bi <dandan.bi@intel.com>,
	Erich McMillan <emcmillan@microsoft.com>,
	Jian J Wang <jian.j.wang@intel.com>,
	Liming Gao <gaoliming@byosoft.com.cn>,
	Star Zeng <star.zeng@intel.com>,
	Zhichao Gao <zhichao.gao@intel.com>,
	Zhiguang Liu <zhiguang.liu@intel.com>,
	Michael Kubacki <michael.kubacki@microsoft.com>
Subject: Re: [edk2-devel] [PATCH v3 01/12] MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
Message-ID: <20230214130114.kp4z4zmfjgaalv47@sirius.home.kraxel.org>
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>
MIME-Version: 1.0
In-Reply-To: <010201864b8f56cb-c9b052f6-c9e6-4c22-9d99-c87c947a7169-000000@eu-west-1.amazonses.com>
X-Scanned-By: MIMEDefang 3.1 on 10.11.54.3
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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