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::144; helo=mail-it1-x144.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x144.google.com (mail-it1-x144.google.com [IPv6:2607:f8b0:4864:20::144]) (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 D155821184ADA for ; Fri, 23 Nov 2018 00:35:30 -0800 (PST) Received: by mail-it1-x144.google.com with SMTP id x124so17321758itd.1 for ; Fri, 23 Nov 2018 00:35:30 -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=SWP0MZjWITVc92PPvTFuOKp1RDHTmTg+JhTVHgGjj5w=; b=O4OpiJAWnf69ar1XJSIVy1SqSVeix2EqnVGR+536xZZkl6w4aLOFwSL8KUNvZ+ecd0 hyMjHX0+l17DeOB3AaJVoD1gY6DBA1gqiePjiza214uPMoe57BS/GC1NRDGQOenMVPwv qhoovDR8joFpgVQuP2gBE78r2Kd/ziXZZW0c4= 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=SWP0MZjWITVc92PPvTFuOKp1RDHTmTg+JhTVHgGjj5w=; b=GhX6ugO5zE7B83/2XL44LchP6Q3IzL3jwdqiZZ/wAfp4mi0GzRAqCrEHVyDp0BQi5/ CYZHWBA8nliUmRBdfKfhYCYCbyuEOm1zvge7TV3gGpKTRACHTRJYYmPyD7EHFbkgejRV TBxiAzankNX7LHzmO2q1HL2ayM9z51dioXFbW4UwUxGczJmeGWLm5YrMaRt9WWecNd5c kIHfnQ8U3ZBGrlFm0rlOeoMTTrHAGcyjAL+3id0FgOhIwavyxCGfUzEsez3WLnwL4VnP swujZ2rQk/iQUWooyJIY/BycrQTIStvtePUaSZ64G7+Gzg4caJBN9xk4ZgVgPMT/HKyV JYDg== X-Gm-Message-State: AGRZ1gLTP82H2lI1ZzqSC9CjEOXYqCj7brnSsbrGjr2LpeTt0IYRwhZZ r9UN0b/xDoJWrAJQYVJXfy34Xbrpwz5Fat5yRwiWGA== X-Google-Smtp-Source: AFSGD/WLqhtjnjNtDg4o6rpkq1PICAIXzpaIlLCuEhzOlVIGkkf7uAx+rhG4G4iOCld99eIQAec05Bf9HqdoF3qb6Rs= X-Received: by 2002:a24:710:: with SMTP id f16mr11017032itf.121.1542962129474; Fri, 23 Nov 2018 00:35:29 -0800 (PST) MIME-Version: 1.0 References: <20181122172645.20819-1-ard.biesheuvel@linaro.org> <20181122172645.20819-5-ard.biesheuvel@linaro.org> In-Reply-To: From: Ard Biesheuvel Date: Fri, 23 Nov 2018 09:35:20 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Thomas Panakamattam Abraham , Nariman Poushin , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= Subject: Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths 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: Fri, 23 Nov 2018 08:35:31 -0000 Content-Type: text/plain; charset="UTF-8" On Thu, 22 Nov 2018 at 19:35, Laszlo Ersek wrote: > > On 11/22/18 18:26, Ard Biesheuvel wrote: > > DevicePath node types may have any size, and so it is up to the > > code that manipulates them to ensure that dereferencing them only > > occurs when the pointer is aligned explicitly. > > > > Since BdsConnectAndUpdateDevicePath() has only two callers, > > at d9e68a756cfb ("Platform/ARM/SgiPkg: increase max variable size to > 8KB", 2018-11-20), it seems to have three callers: > > - itself > - BdsConnectDevicePath() > - BdsLoadImageAndUpdateDevicePath() > Indeed. I am updating the second patch to get rid of everything in BdsLib we are not currently using. > > one of > > which itself, we can simply duplicate the device path (similar to > > how DxeCore's CoreConnectController () does it), and free the pool > > allocation again on the way out. (Note that the allocation only > > occurs when the non-recursive path is taken) > > I think this rather works around than fixes the problem -- just because > every remaining device path "slice" is realigned as we advance, it's not > guaranteed that any and all CHAR16 fields in the now-first node will be > naturally aligned. > > ... However, it certainly applies to FILEPATH_DEVICE_PATH.PathName, > which is likely the only such field that we care about. :) > Looking at 56bed2f41022afcbadecc9f2d537bd31c3d44cbc ("^W never mind ... the intent appears to be that device path struct members do appear naturally aligned, even if the size of the data structure is not a multiple of the max alignment we expect to encounter. Presumably, this is why CoreConnectController () does the same in this regard. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > Platform/ARM/Library/BdsLib/BdsFilePath.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/Platform/ARM/Library/BdsLib/BdsFilePath.c b/Platform/ARM/Library/BdsLib/BdsFilePath.c > > index 74fdbbee773d..543ac8f83086 100644 > > --- a/Platform/ARM/Library/BdsLib/BdsFilePath.c > > +++ b/Platform/ARM/Library/BdsLib/BdsFilePath.c > > @@ -421,7 +421,7 @@ BdsConnectAndUpdateDevicePath ( > > } > > > > if (RemainingDevicePath) { > > - *RemainingDevicePath = Remaining; > > + *RemainingDevicePath = DuplicateDevicePath (Remaining); > > } > > > > return Status; > > OK, so this makes BdsConnectAndUpdateDevicePath()'s RemainingDevicePath > output param dynamically allocated. And this change works fine with the > recursive logic too, as you say in the commit message. > Yep. > > @@ -1333,14 +1333,18 @@ BdsLoadImageAndUpdateDevicePath ( > > } > > We already need some error handling here. The control flow in > BdsConnectAndUpdateDevicePath() boggles my mind a bit, but I think it > can output a dynamically allocated RemainingDevicePath *and* return an > error. > > Namely, assume that TryRemovableDevice() is reached, and it fails. > That doesn't make sense. I'll update that routine to only do the clone if it returns EFI_SUCCESS. > So, I think we should add an error handling label > ("FreeRemainingDevicePath"), and jump to it, from both first "return" > statements in this function. > > Also, we should likely set RemainingDevicePath to NULL at the top of the > function, and check it at the end, because... ugh... > BdsConnectAndUpdateDevicePath() might also fail without assigning > *RemainingDevicePath? > The above change should fix that as well afaict. > > > > FileLoader = FileLoaders; > > + Status = EFI_UNSUPPORTED; > > while (FileLoader->Support != NULL) { > > if (FileLoader->Support (*DevicePath, Handle, RemainingDevicePath)) { > > - return FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, Type, Image, FileSize); > > + Status = FileLoader->LoadImage (DevicePath, Handle, RemainingDevicePath, > > + Type, Image, FileSize); > > + break; > > } > > FileLoader++; > > } > > > > - return EFI_UNSUPPORTED; > > + FreePool (RemainingDevicePath); > > + return Status; > > } > > > > EFI_STATUS > > > > As I mention near the commit message, BdsConnectDevicePath() is not > updated. Is that OK? ... Oh wait, BdsConnectDevicePath() is not called > by anything. Append another patch to drop it, like > BdsStartEfiApplication()? Then this patch will be fine, assuming you add > the "goto"s. > > Thanks! > Laszlo