From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::22a; helo=mail-io0-x22a.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x22a.google.com (mail-io0-x22a.google.com [IPv6:2607:f8b0:4001:c06::22a]) (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 D14252095B09C for ; Fri, 13 Oct 2017 00:29:34 -0700 (PDT) Received: by mail-io0-x22a.google.com with SMTP id 101so8056393ioj.3 for ; Fri, 13 Oct 2017 00:33:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=TUDDI7sR2ZMpAiIbPTVa5pcIrzzbe/E4/D9C4U9XM48=; b=OOga6jOxYpFzGTMTaRfwfH22ocB7DPXoxLWvC04NGe2au7ZgoVcpXsonea3Q8hy6VF xtBh4HPGmLvWAigjPoN06WLopGGwjNZ9rYfc0RcCTGZscdZduiLjQ/8iYCeCKpGPTrQO jovqxSMZWf0Lqd1mSfhQHWOIHBQ21j0S2LpTk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=TUDDI7sR2ZMpAiIbPTVa5pcIrzzbe/E4/D9C4U9XM48=; b=H95Z6O794sA45CxBgsx8nz2KjwHVa0QK7hrp7kBSsMYyx6Yw3JlwVwQCbty+uf4BUO Z4TIObtnpSZdU8zjkuM7xmmiQMqBeLjMCYGMVQx7imp458Tz0J+8Xxz+to7erU8x0nb3 TtUOPDqCMe46mQ4Ns/4j5s2xUjhipymbJnEg7ZCCINQJJpH550zx/VA3vnTnuh9cBJcu dNS/yUlqRnAmN+mMC4Py2dESboTwJQTt+KUP1GWdfk4LWlFo1XDF5bHnsaQpSY/gUs92 +PVVtMyzb09yAxHbJ+5shnH1SE6nPKBXl1HGk+itR7+OzKFobeRstSB2GKaop3VAxPBL 3nYg== X-Gm-Message-State: AMCzsaWRcYipx9eIEMjyPuO0HQzLQ51OfJSz8kjj8mOUOmTB2Ey1bL1w cFPMUMPuwGVDJQCHKlCVNCNBLIdpAraAE4hM/0xdRg== X-Google-Smtp-Source: ABhQp+TYkEYU4AWGwKTtQ+CMepyAcHlIK3iTgRqqmBI42HmaEbGsUr5ToEAjtJi9Njvn3e8rSrri0XL/NBDJU2l6HYY= X-Received: by 10.107.154.138 with SMTP id c132mr691683ioe.95.1507879985035; Fri, 13 Oct 2017 00:33:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.131.167 with HTTP; Fri, 13 Oct 2017 00:33:04 -0700 (PDT) In-Reply-To: <20170926201529.11644-5-evan.lloyd@arm.com> References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-5-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Fri, 13 Oct 2017 08:33:04 +0100 Message-ID: To: Evan Lloyd Cc: "edk2-devel@lists.01.org" , <"ard.biesheuvel@linaro.org"@arm.com>, <"leif.lindholm@linaro.org"@arm.com>, <"Matteo.Carlini@arm.com"@arm.com>, <"nd@arm.com"@arm.com> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Oct 2017 07:29:35 -0000 Content-Type: text/plain; charset="UTF-8" On 26 September 2017 at 21:15, wrote: > From: Girish Pathak > > This change adds some debug assertions e.g to catch NULL pointer errors > missing in PL11Lcd and HdLcd modules. > > This change also improves related error handling code. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Girish Pathak > Signed-off-by: Evan Lloyd > --- > ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c | 44 ++++++++++++++++++-- > ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 43 ++++++++++++++++++- > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c | 8 ++-- > ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c | 8 ++-- > 4 files changed, 90 insertions(+), 13 deletions(-) > > diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c > index b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a71d0c7cce72d71c6da5 100644 > --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c > +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay ( > * buffer in bytes > * > * @retval EFI_SUCCESS Frame buffer memory allocation success. > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize are NULL. > * @retval !(EFI_SUCCESS) Other errors. > **/ > EFI_STATUS > @@ -151,6 +152,13 @@ LcdPlatformGetVram ( > EFI_STATUS Status; > EFI_ALLOCATE_TYPE AllocationType; > > + // Check VramBaseAddress and VramSize are not NULL. > + if (VramBaseAddress == NULL || VramSize == NULL) { > + ASSERT (VramBaseAddress != NULL); > + ASSERT (VramSize != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > // Set the vram size > *VramSize = LCD_VRAM_SIZE; > > @@ -169,6 +177,7 @@ LcdPlatformGetVram ( > VramBaseAddress > ); > if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > return Status; > } > > @@ -179,8 +188,8 @@ LcdPlatformGetVram ( > *VramSize, > EFI_MEMORY_WC > ); > - ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); What is the point of this change? > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); > return Status; > } > @@ -215,6 +224,7 @@ LcdPlatformSetMode ( > EFI_STATUS Status; > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > return EFI_INVALID_PARAMETER; > } > > @@ -264,6 +274,7 @@ LcdPlatformSetMode ( > * > * @retval EFI_SUCCESS Success if the requested mode is found. > * @retval EFI_INVALID_PARAMETER Requested mode not found. > + * @retval EFI_INVALID_PARAMETER Info is NULL. > **/ > EFI_STATUS > LcdPlatformQueryMode ( > @@ -271,7 +282,9 @@ LcdPlatformQueryMode ( > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info > ) > { > - if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); Please don't put anything that may have a side effect inside ASSERT (), since they are dropped from RELEASE builds. You can just use ASSERT (FALSE) here, or use a temp variable. > + ASSERT (Info != NULL); > return EFI_INVALID_PARAMETER; > } > > @@ -334,6 +347,28 @@ LcdPlatformGetTimings ( > ) > { > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); same here > + return EFI_INVALID_PARAMETER; > + } > + > + if (HRes == NULL > + || HSync == NULL > + || HBackPorch == NULL > + || HFrontPorch == NULL > + || VRes == NULL > + || VSync == NULL > + || VBackPorch == NULL > + || VFrontPorch == NULL) > + { > + // One of the pointers is NULL > + ASSERT (HRes != NULL); > + ASSERT (HSync != NULL); > + ASSERT (HBackPorch != NULL); > + ASSERT (HFrontPorch != NULL); > + ASSERT (VRes != NULL); > + ASSERT (VSync != NULL); > + ASSERT (VBackPorch != NULL); > + ASSERT (VFrontPorch != NULL); > return EFI_INVALID_PARAMETER; > } > > @@ -356,6 +391,7 @@ LcdPlatformGetTimings ( > * > * @retval EFI_SUCCESS The requested mode is found. > * @retval EFI_INVALID_PARAMETER Requested mode not found. > + * @retval EFI_INVALID_PARAMETER Bpp is NULL. > **/ > EFI_STATUS > LcdPlatformGetBpp ( > @@ -363,7 +399,9 @@ LcdPlatformGetBpp ( > OUT LCD_BPP * CONST Bpp > ) > { > - if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > + ASSERT (Bpp != NULL); > return EFI_INVALID_PARAMETER; > } > > diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > index 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690af7233c1cebfe1ad339b 100644 > --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay ( > * buffer in bytes > * > * @retval EFI_SUCCESS Frame buffer memory allocation success. > + * @retval EFI_INVALID_PARAMETER VramBaseAddress or VramSize is NULL. > * @retval !(EFI_SUCCESS) Other errors. > **/ > EFI_STATUS > @@ -203,6 +204,13 @@ LcdPlatformGetVram ( > > Status = EFI_SUCCESS; > > + // Check VramBaseAddress and VramSize are not NULL. > + if (VramBaseAddress == NULL || VramSize == NULL) { > + ASSERT (VramBaseAddress != NULL); > + ASSERT (VramSize != NULL); > + return EFI_INVALID_PARAMETER; > + } > + > // Is it on the motherboard or on the daughterboard? > switch (PL111_CLCD_SITE) { > > @@ -223,6 +231,7 @@ LcdPlatformGetVram ( > VramBaseAddress > ); > if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > return Status; > } > > @@ -233,8 +242,8 @@ LcdPlatformGetVram ( > *VramSize, > EFI_MEMORY_WC > ); > - ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); Please drop this change > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); > return Status; > } > @@ -293,6 +302,7 @@ LcdPlatformSetMode ( > UINT32 SysId; > > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); No function calls inside ASSERT () please > return EFI_INVALID_PARAMETER; > } > > @@ -359,6 +369,7 @@ LcdPlatformSetMode ( > * (on success). > * > * @retval EFI_SUCCESS Success if the requested mode is found. > + * @retval EFI_INVALID_PARAMETER Info is NULL. > * @retval EFI_INVALID_PARAMETER Requested mode not found. > **/ > EFI_STATUS > @@ -367,7 +378,9 @@ LcdPlatformQueryMode ( > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info > ) > { > - if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > + ASSERT (Info != NULL); > return EFI_INVALID_PARAMETER; > } > > @@ -415,6 +428,7 @@ LcdPlatformQueryMode ( > * > * @retval EFI_SUCCESS Success if the requested mode is found. > * @retval EFI_INVALID_PARAMETER Requested mode not found. > + * @retval EFI_INVALID_PARAMETER One of the OUT parameters is NULL. > **/ > EFI_STATUS > LcdPlatformGetTimings ( > @@ -430,6 +444,28 @@ LcdPlatformGetTimings ( > ) > { > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > + return EFI_INVALID_PARAMETER; > + } > + > + if (HRes == NULL > + || HSync == NULL > + || HBackPorch == NULL > + || HFrontPorch == NULL > + || VRes == NULL > + || VSync == NULL > + || VBackPorch == NULL > + || VFrontPorch == NULL) > + { > + // One of the pointers is NULL > + ASSERT (HRes != NULL); > + ASSERT (HSync != NULL); > + ASSERT (HBackPorch != NULL); > + ASSERT (HFrontPorch != NULL); > + ASSERT (VRes != NULL); > + ASSERT (VSync != NULL); > + ASSERT (VBackPorch != NULL); > + ASSERT (VFrontPorch != NULL); > return EFI_INVALID_PARAMETER; > } > > @@ -452,6 +488,7 @@ LcdPlatformGetTimings ( > * > * @retval EFI_SUCCESS The requested mode is found. > * @retval EFI_INVALID_PARAMETER Requested mode not found. > + * @retval EFI_INVALID_PARAMETER Bpp is NULL. > **/ > EFI_STATUS > LcdPlatformGetBpp ( > @@ -460,6 +497,8 @@ LcdPlatformGetBpp ( > ) > { > if (ModeNumber >= LcdPlatformGetMaxMode ()) { > + ASSERT (ModeNumber < LcdPlatformGetMaxMode ()); > + ASSERT (Bpp != NULL); > return EFI_INVALID_PARAMETER; > } > > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c > index 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8ddb0cf7cbfe59d2f4dc49c 100644 > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c > @@ -109,15 +109,15 @@ LcdSetMode ( > &VBackPorch, > &VFrontPorch > ); > - ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > - return EFI_DEVICE_ERROR; > + ASSERT_EFI_ERROR (Status); > + return Status; > } > > Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp); > - ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > - return EFI_DEVICE_ERROR; > + ASSERT_EFI_ERROR (Status); > + return Status; > } > > BytesPerPixel = GetBytesPerPixel (LcdBpp); > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c > index 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4ddab3898f6e0dbf9a1572 100644 > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c > @@ -110,15 +110,15 @@ LcdSetMode ( > &VBackPorch, > &VFrontPorch > ); > - ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > - return EFI_DEVICE_ERROR; > + ASSERT_EFI_ERROR (Status); > + return Status; > } > > Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp); > - ASSERT_EFI_ERROR (Status); > if (EFI_ERROR (Status)) { > - return EFI_DEVICE_ERROR; > + ASSERT_EFI_ERROR (Status); > + return Status; > } > > // Disable the CLCD_LcdEn bit > -- > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >