linux-kernel - Re: [PATCH 1/1] efi/libstub: describe efi_relocate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Thu, 20 Feb 2020 08:52:48 +0100
From:   Ard Biesheuvel <ardb@...nel.org>
To:     Randy Dunlap <rdunlap@...radead.org>
Cc:     Heinrich Schuchardt <xypron.glpk@....de>,
        linux-efi <linux-efi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] efi/libstub: describe efi_relocate_kernel()

On Thu, 20 Feb 2020 at 08:02, Randy Dunlap <rdunlap@...radead.org> wrote:
>
> Hi,
> Mostly looks good.  One comment below:
>
> On 2/19/20 10:53 PM, Heinrich Schuchardt wrote:
> > Update the description of of efi_relocate_kernel() to match Sphinx style.
> >
> > Update parameter references in the description of other memory functions
> > to use @param style.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@....de>
> > ---
> >  drivers/firmware/efi/libstub/mem.c | 38 +++++++++++++++++++-----------
> >  1 file changed, 24 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/mem.c b/drivers/firmware/efi/libstub/mem.c
> > index 0d57078e5e62..7efe3ed2d5a6 100644
> > --- a/drivers/firmware/efi/libstub/mem.c
> > +++ b/drivers/firmware/efi/libstub/mem.c
> > @@ -86,7 +86,7 @@ efi_status_t efi_get_memory_map(struct efi_boot_memmap *map)
> >   *
> >   * Allocate pages as EFI_LOADER_DATA. The allocated pages are aligned according
> >   * to EFI_ALLOC_ALIGN. The last allocated page will not exceed the address
> > - * given by 'max'.
> > + * given by @max.
> >   *
> >   * Return:   status code
> >   */
> > @@ -126,10 +126,10 @@ efi_status_t efi_allocate_pages(unsigned long size, unsigned long *addr,
> >   * @addr:    on exit the address of the allocated memory
> >   * @min:     minimum address to used for the memory allocation
> >   *
> > - * Allocate at the lowest possible address that is not below 'min' as
> > - * EFI_LOADER_DATA. The allocated pages are aligned according to 'align' but at
> > + * Allocate at the lowest possible address that is not below @min as
> > + * EFI_LOADER_DATA. The allocated pages are aligned according to @align but at
> >   * least EFI_ALLOC_ALIGN. The first allocated page will not below the address
> > - * given by 'min'.
> > + * given by @min.
> >   *
> >   * Return:   status code
> >   */
> > @@ -214,7 +214,7 @@ efi_status_t efi_low_alloc_above(unsigned long size, unsigned long align,
> >   * @addr:    start of the memory area to free (must be EFI_PAGE_SIZE
> >   *           aligned)
> >   *
> > - * 'size' is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
> > + * @size is rounded up to a multiple of EFI_ALLOC_ALIGN which is an
> >   * architecture specific multiple of EFI_PAGE_SIZE. So this function should
> >   * only be used to return pages allocated with efi_allocate_pages() or
> >   * efi_low_alloc_above().
> > @@ -230,15 +230,25 @@ void efi_free(unsigned long size, unsigned long addr)
> >       efi_bs_call(free_pages, addr, nr_pages);
> >  }
> >
> > -/*
> > - * Relocate a kernel image, either compressed or uncompressed.
> > - * In the ARM64 case, all kernel images are currently
> > - * uncompressed, and as such when we relocate it we need to
> > - * allocate additional space for the BSS segment. Any low
> > - * memory that this function should avoid needs to be
> > - * unavailable in the EFI memory map, as if the preferred
> > - * address is not available the lowest available address will
> > - * be used.
> > +/**
> > + * efi_relocate_kernel() - copy memory area
> > + * @image_addr:              address of memory area to copy, on exit target address
>
> The "on exit target address" is a little bit confusing IMO.
> Is it like this?
>
>   On exit, @image_addr is updated to the target copy address that was used.
>

Agreed.

> ?  or some other better description?
>
> Thanks.
>
> Acked-by: Randy Dunlap <rdunlap@...radead.org>
>

Applied to efi/next with Randy's suggestion folded in

Heinrich: I folded the first few hunks into the respective changes of
yours that were already queued up

Thanks,


>
> > + * @image_size:              size of memory area to copy
> > + * @alloc_size:              minimum size of memory to allocate, must be greater or
> > + *                   equal to image_size
> > + * @preferred_addr:  preferred target address
> > + * @alignment:               minimum alignment of the allocated memory area. It
> > + *                   should be a power of two.
> > + * @min_addr:                minimum target address
> > + *
> > + * Copy a memory area to a newly allocated memory area aligned according
> > + * to @alignment but at least EFI_ALLOC_ALIGN. If the preferred address
> > + * is not available, the allocated address will not be below @min_addr.
> > + *
> > + * This function is used to copy the Linux kernel verbatim. It does not apply
> > + * any relocation changes.
> > + *
> > + * Return:           status code
> >   */
> >  efi_status_t efi_relocate_kernel(unsigned long *image_addr,
> >                                unsigned long image_size,
> > --
> > 2.25.0
> >
>
>
> --
> ~Randy
>