public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>, edk2-devel@lists.01.org
Cc: leif.lindholm@linaro.org, thomas.abraham@arm.com,
	nariman.poushin@linaro.org, philmd@redhat.com
Subject: Re: [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths
Date: Thu, 22 Nov 2018 19:35:35 +0100	[thread overview]
Message-ID: <d160621c-c684-0a60-1ab5-dfafae028e46@redhat.com> (raw)
In-Reply-To: <20181122172645.20819-5-ard.biesheuvel@linaro.org>

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()

> 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. :)

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  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.

> @@ -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.

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?

>  
>    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


  reply	other threads:[~2018-11-22 18:35 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-22 17:26 [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Ard Biesheuvel
2018-11-22 17:26 ` [PATCH edk2-platforms 1/4] Platform/ARM: import ARM platform specific BdsLib header Ard Biesheuvel
2018-11-22 17:36   ` Laszlo Ersek
2018-11-22 17:26 ` [PATCH edk2-platforms 2/4] Platform/ARM/BdsLid: drop unused BdsStartEfiApplication () Ard Biesheuvel
2018-11-22 17:55   ` Laszlo Ersek
2018-11-22 17:26 ` [PATCH edk2-platforms 3/4] Platform/ARM/BdsLib: don't clobber BdsLoadImage() DevicePath IN param Ard Biesheuvel
2018-11-22 18:09   ` Laszlo Ersek
2018-11-22 18:14     ` Ard Biesheuvel
2018-11-22 18:23       ` Laszlo Ersek
2018-11-22 17:26 ` [PATCH edk2-platforms 4/4] Platform/ARM/BdsLib: maintain alignment for DevicePaths Ard Biesheuvel
2018-11-22 18:35   ` Laszlo Ersek [this message]
2018-11-23  8:35     ` Ard Biesheuvel
2018-11-23  9:39       ` Laszlo Ersek
2018-11-23  4:20 ` [PATCH edk2-platforms 0/4] Platform/ARM: fix DevicePath mishandling in BdsLib Thomas Abraham
2018-11-23  8:44   ` Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d160621c-c684-0a60-1ab5-dfafae028e46@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox