On Thu, 18 Jun, at 11:27:55PM, Michael Brown wrote:
> On 18/06/15 23:02, Matt Fleming wrote:
> >On Tue, 16 Jun, at 11:37:25AM, Linn Crosetto wrote:
> >>I have been reverting this patch as a workaround. The fields need to be changed,
> >>but I am not that familiar with the code. Maybe there is a way to use a
> >>heuristic to calculate the best values based on init_sz?
> >
> >Linn, could you please provide some details of the system that you're
> >booting this kernel on? EDK2 does not include any checks for this
> >alignment requirement, which probably explains why no one else ever
> >caught this issue.
> >
> >I can't think of any way to fix this without simply doing a revert of
> >commit aeffc4928ea2 ("x86/efi: Request desired alignment via the PE/COFF
> >headers"). Especially since that patch was an optimisation and not a bug
> >fix.
>
> I'm pretty sure that patch _is_ a bug fix, not just an optimisation.
> It looks as though the commit log message was changed from what I
> originally wrote:
>
> The kernel will align itself to the nearest boundary specified by the
> kernel_alignment field in the bzImage header. If the kernel is loaded
> to an address which is not sufficiently aligned, it will therefore use
> memory beyond that indicated solely by the init_size field.
>
> The PE/COFF headers now include a .bss section to describe the
> requirements of the init_size field, but do not currently expose the
> alignment requirement. Consequently, a kernel loaded via the PE entry
> point may still end up overwriting unexpected areas of memory.
>
> to
>
> The EFI boot stub goes to great pains to relocate the kernel image to
> an appropriately aligned address, as indicated by the ->kernel_alignment
> field in the bzImage header. However, for the PE stub entry case, we
> can request that the EFI PE/COFF loader do the work for us.
>
> If the patch is reverted, then I think it will cause undefined
> behaviour on some platforms (which happen to load the kernel to
> non-preferred alignment, and where the memory immediately after the
> loaded kernel happens to be in use for something).
I thought that we had previously established that this wasn't true?
On Fri, 11 Jul, at 01:18:43AM, Michael Brown wrote:
> > Is this actually true? There is code within the EFI boot stub to
> > allocate space for the kernel image and perform the relocation if it's
> > not already suitably aligned.
> >
> > Or is the above paragraph referring to the previously merged patch?
>
> The "...headers now include..." part was referring to the previously
> merged patch to add the .bss section.
>
> I haven't actually looked at the code which performs the alignment; I
> was going on hpa's concern that merely exposing init_size would be
> insufficient due to the potential for alignment. My understanding
> (possibly incorrect) was that the alignment was carried out using
> something simple along the lines of:
>
> new_kernel_start = align ( kernel_start, kernel_alignment );
> memmove ( new_kernel_start, kernel_start, kernel_len );
>
> i.e. that the memory used for alignment was not explicitly allocated.
> If the EFI boot stub instead allocates space for the aligned kernel
> using AllocatePages() (and allocates enough space for the whole of
> init_size), then the problem I described does not exist.
To which I replied with,
> Right, this shouldn't be a problem because we do in fact allocate space
> using the EFI boottime services in efi_relocate_kernel(), taking the
> alignment into account, and then perform the kernel image copy.
>
> I still think your change makes sense, I'm just inclined to delete the
> paragraph referring to the corruption bug (which we've established
> doesn't exist).
Do we still have a bug?
--
Matt Fleming, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
