From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 D9B14211982DC for ; Mon, 3 Dec 2018 05:05:41 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 55D1B81DE9; Mon, 3 Dec 2018 13:05:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-61.rdu2.redhat.com [10.10.120.61]) by smtp.corp.redhat.com (Postfix) with ESMTP id C523917BA2; Mon, 3 Dec 2018 13:05:39 +0000 (UTC) To: Ard Biesheuvel , edk2-devel@lists.01.org Cc: Yonghong Zhu , Liming Gao , Bob Feng , Jaben Carsey References: <20181130224537.18936-1-ard.biesheuvel@linaro.org> <20181130224537.18936-5-ard.biesheuvel@linaro.org> From: Laszlo Ersek Message-ID: Date: Mon, 3 Dec 2018 14:05:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20181130224537.18936-5-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Mon, 03 Dec 2018 13:05:41 +0000 (UTC) Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 03 Dec 2018 13:05:42 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 11/30/18 23:45, Ard Biesheuvel wrote: > Replace the default size limit of IsDevicePathValid() with a value > that does not depend on the native word size of the build host. > > 64 KB seems sufficient as the upper bound of a device path handled > by UEFI. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ard Biesheuvel > Reviewed-by: Jaben Carsey > --- > BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c > index d4ec2742b7c8..ba7f83e53070 100644 > --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c > +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c > @@ -62,7 +62,7 @@ IsDevicePathValid ( > ASSERT (DevicePath != NULL); > > if (MaxSize == 0) { > - MaxSize = MAX_UINTN; > + MaxSize = MAX_UINT16; > } > > // > @@ -78,7 +78,7 @@ IsDevicePathValid ( > return FALSE; > } > > - if (NodeLength > MAX_UINTN - Size) { > + if (NodeLength > MAX_UINT16 - Size) { > return FALSE; > } > Size += NodeLength; > I'm somewhat undecided about this patch. (1) IsDevicePathValid() also exists in: - MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c - MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c Both have: if (MaxSize == 0) { MaxSize = MAX_UINTN; } Relative to those, this change departs quite strongly. (2) In addition, a single device path node may extend up to 64KB. That would be pathologic, yes, but the option is there. ... Of course, we are discussing theoretical limits. Still I'd feel more comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't cost us anything (in development effort), it would be a no-op on 32-bit build hosts, it would be a theoretical-only change on 64-bit build hosts, and it would leave us with a larger "safety margin". I won't insist, but I thought I should raise this. (Sorry if this has been discussed under v1 already.) If you agree, no need to repost (from my side anyway) just for this. With or without the update: Reviewed-by: Laszlo Ersek Thanks Laszlo