Re: [PATCH 4.19 84/86] efi/x86: Handle by-ref arguments covering multiple pages in mixed mode

[Ard Biesheuvel]

On Wed, 11 Mar 2020 at 09:28, Pavel Machek <pavel@xxxxxxx> wrote:
>
> On Wed 2020-03-11 14:13:11, Greg Kroah-Hartman wrote:
> > On Wed, Mar 11, 2020 at 02:01:07PM +0100, Pavel Machek wrote:
> > > Hi!
> > >
> > > > Currently, the mixed mode runtime service wrappers require that all by-ref
> > > > arguments that live in the vmalloc space have a size that is a power of 2,
> > > > and are aligned to that same value. While this is a sensible way to
> > > > construct an object that is guaranteed not to cross a page boundary, it is
> > > > overly strict when it comes to checking whether a given object violates
> > > > this requirement, as we can simply take the physical address of the first
> > > > and the last byte, and verify that they point into the same physical
> > > > page.
> > >
> > > Dunno. If start passing buffers that _sometime_ cross page boundaries,
> > > we'll get hard to debug failures. Maybe original code is better
> > > buecause it catches problems earlier?
> > >
> > > Furthermore, all existing code should pass aligned, 2^n size buffers,
> > > so we should not need this in stable?
> >
> > For some crazy reason you cut out the reason I applied this patch to the
> > stable tree.  From the changelog text:
> >       Fixes: f6697df36bdf0bf7 ("x86/efi: Prevent mixed mode boot
> >corruption with CONFIG_VMAP_STACK=y")
>
> I did not notice that, but reviewing f669 does not really help. If
> there is some known code that passes unaligned (but guaranteed
> not-to-cross-page) buffers here, then yes, but is it? Having
> not-page-crossing guarantees is kind of hard without alignment.
>
> People seem to be adding Fixes: tags even if it is not a bugfix, just
> as reminder that this has relation to some other commit...
>

If you read the commit log until the end, you will find a paragraph
that describes how the old code warns about, but then ignores the
error condition, and proceeds in a way that may cause data corruption.
Also, on x86, the stack alignment is only 8 bytes, so with the
introduction of vmapped stacks, most guarantees about alignment of
objects in the vmalloc space went out the window, potentially
triggering this condition in unanticipated ways.

It is not very constructive to comment on 'what people seem to be
doing' if you have no clue what the context of the change is. Opinions
are welcome but informed ones are preferred.


>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany