Re: New EFI thunk alignment WARN_ON in 5.6 triggers multiple times

[Arvind Sankar <nivedita@alum.mit.edu>]

On Wed, Feb 12, 2020 at 04:00:24PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 2/12/20 12:53 PM, Ard Biesheuvel wrote:
> > 
> > 
> > It seems that the purpose of the alignment check is to ensure that the
> > data does not cross a page boundary, so that the data is guaranteed to
> > be contiguous in the physical address space as well.
> > 
> > So in that light, the fix is most definitely wrong, although I am not
> > sure how this is supposed to work generally.
> 
> I'm not sure that is what it is trying to check, if that is what it is
> trying to check then the code is certainly wrong.
> 
> Let me first quote the entire check:
> 
>          /*
>           * A fully aligned variable on the stack is guaranteed not to
>           * cross a page bounary. Try to catch strings on the stack by
>           * checking that 'size' is a power of two.
>           */
>          bad_size = size > PAGE_SIZE || !is_power_of_2(size);
> 
>          WARN_ON(!IS_ALIGNED((unsigned long)va, size) || bad_size);
> 
> AFAIK EFI is using the identity map, and the kernel stack is
> physically contiguous, so crossing a page boundary should not a
> problem. Also notice how the bad_size thing is talking about
> page boundary crossing, but the thing triggering is the
> IS_ALIGNED check. AFAIK there is no requirement for a struct, e.g.
> an UUID (which is the problem here) to be aligned to its size,
> it just needs to be 8 byte / 64 bit aligned, which it is yet
> the IS_ALIGNED check is failing because it is checking for
> a larger, in this case twice as large, but possibly it will
> end up checking for a much larger alignment.
> 
> As said I'm not exactly familiar with the alignment requirements
> but the current check certainly seems wrong.

If VMAP_STACK is enabled, the stack pages may not be physically
contiguous. So the check is trying to warn about all cases where the
object might cross a page boundary and so not be physically contiguous,
even if the current call is ok because it doesn't cross the page
boundary.

It should probably also error out rather than making the call if it is
actually the case that it spans non-physically contiguous pages.

However, I'm also getting confused about how mixed-mode works at all if
we have more than 4Gb RAM, which it seems we want to support as we take
pains to allocate a stack below 4Gb for the thunk. But in this case, the
kernel text and stack could be above 4Gb physically, so rather than
using a 1:1 map we'd need to find some space in the low 4Gb of virtual
addresses to map those, but I don't see where we do this? Also, that
dynamically allocated variable_name could be above 4G as well.