From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::d42; helo=mail-io1-xd42.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8B4622118F78B for ; Tue, 4 Dec 2018 23:42:14 -0800 (PST) Received: by mail-io1-xd42.google.com with SMTP id t24so15929448ioi.0 for ; Tue, 04 Dec 2018 23:42:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=fLO8kqXhxrBSmCE5dPcrh5M3PLydddNH2bGEVLzbiZE=; b=Lp2mcb70Y8Cv5uP/86eqYiXIHUODHNFH/Yq9+vfqps+c5Yjxhk/OViLA3JS5S537kl nDfDkDDlrBVK9S51szRXxaIRCnu/NQj9o0kvItXrrnt0L/VBKrz8YJ1WalTOvb+fXYPP jOTxsPdqMvj2i6oh/w1/OEftj24i77L5LxxM8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=fLO8kqXhxrBSmCE5dPcrh5M3PLydddNH2bGEVLzbiZE=; b=TT5vZk/Ijvd/TMZlc/yhaInxChpq+N06cGE9aVskDAglA25eYI8Lp/KW8vqR0qUgbK Ld/xf4t0OQNspqgFxa7BdH2aH7+bUai+fdzGgxqVaWYtI2f8LhWaH1ktsgYYUIhmSZA4 qZ1L2AgdxpC8kDhUE8altbBDntvgdL4e/xw9dnQfJelJj0J7zRBcyVM10Y7aho+K8xBy BBCeBEZXmP+B5LLildJX9Mp05b6lIf2rVsiGgXHLtQeXzUDXYSujmqkawBQ0h7Y63QZF voGGHyNSUa7w+9EU6k33n1/EhZgDcQVuUhiSlevUk3BPKj4/nuAFU1et7VCMl8NVCI9A cCWg== X-Gm-Message-State: AA+aEWbNmXfX/CweJJNkhsLIsP2b4A90f1yeUNsMJPuGaEqm7G3eV6A1 Fnmug+tqt7ZN6JG+1jeKW0WAp6mtrcjQwAPt6/E70A== X-Google-Smtp-Source: AFSGD/UdT8XO4FqM6lawTQfegwqnzliN6bLSJivFcY6GXAJ3Byemwf5xMixPxo9PZ6S6RNHh4S8jsIsWROd2nuRX20I= X-Received: by 2002:a6b:5d01:: with SMTP id r1mr19036233iob.170.1543995733244; Tue, 04 Dec 2018 23:42:13 -0800 (PST) MIME-Version: 1.0 References: <20181130224537.18936-1-ard.biesheuvel@linaro.org> <20181130224537.18936-5-ard.biesheuvel@linaro.org> <4A89E2EF3DFEDB4C8BFDE51014F606A14E383BB7@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E383BB7@SHSMSX104.ccr.corp.intel.com> From: Ard Biesheuvel Date: Wed, 5 Dec 2018 08:42:01 +0100 Message-ID: To: "Gao, Liming" Cc: Laszlo Ersek , "edk2-devel@lists.01.org" , "Zhu, Yonghong" , "Feng, Bob C" , "Carsey, Jaben" 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: Wed, 05 Dec 2018 07:42:14 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, 5 Dec 2018 at 01:04, Gao, Liming wrote: > > Laszlo: > I agree with you. MAX_UINT32 is more comfortable. > Liming, No definitions for MAX_UINT32 exist currently in BaseTools, so I will have to add the following: diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h index b1c6c00a3478..1c40180329c4 100644 --- a/BaseTools/Source/C/Common/CommonLib.h +++ b/BaseTools/Source/C/Common/CommonLib.h @@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. #define MAX_LONG_FILE_PATH 500 #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL) +#define MAX_UINT32 ((UINT32)0xFFFFFFFFULL) #define MAX_UINT16 ((UINT16)0xFFFF) #define MAX_UINT8 ((UINT8)0xFF) #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0])) Does your Reviewed-by cover that as well? > >-----Original Message----- > >From: Laszlo Ersek [mailto:lersek@redhat.com] > >Sent: Monday, December 03, 2018 9:06 PM > >To: Ard Biesheuvel ; edk2-devel@lists.01.org > >Cc: Zhu, Yonghong ; Gao, Liming > >; Feng, Bob C ; Carsey, Jaben > > > >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as > >default device path max size > > > >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