On 11/5/22 23:43, Ard Biesheuvel wrote:
On Sat, 5 Nov 2022 at 22:54, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
(cc Heinrich and Ilias)
On Sat, 5 Nov 2022 at 21:27, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
On Sat, Nov 5, 2022 at 1:18 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
Yeah just rip it out. In the beginning, we tended to make these
warnings noisy so people will actually notice.
Rip it out entirely, or replace ith pr_warn_once()?
A warning that can only trigger on 16k or 64k page size kernels
clearly doesn't have a lot of coverage, so either we just drop it, or
we make the warning use SZ_64K and not PAGE_SIZE.
And if we keep the warning, it should be separate from the if(): when
the regions are misaligned, we have to use RWX mappings because an
adjacent region that gets covered by the same mapping might require
it.
Maybe I'll just whip up a patch myself.
I'd still like to see a memory map (boot with efi=debug) so we can get
this reported and fixed in uboot. We need that so 16k and 64k pages
boot doesn't cause surprises with overlapping mappings.
Here's the dmesg attached with efi=debug for your viewing pleasure.
Thanks.
I've cc'ed the u-boot EFI maintainers, who take EFI spec compliance
very seriously, so I'm sure we'll get this fixed quickly.
Grrr looking at the spec, it seems the wording we proposed at the time
never made it in, and at the moment, it just reads:
Could you, please, provide a link to the proposal.
Best regards
Heinrich
"""
If a 64KiB physical page contains any 4KiB page with any of the
following types listed below, then all 4KiB pages in the 64KiB page
must use identical ARM Memory Page Attributes:
— EfiRuntimeServicesCode
— EfiRuntimeServicesData
...
"""
The problem here is that it doesn't take permission attributes into
account, allowing the firmware to cram code and data regions into the
same 64k page, and instructing the OS to use R-X for the code and RW-
for the data, which it cannot do if it uses 16k or 64k pages.
So let's drop the warning and add it back later once the spec actually
supports it.
Patch on its way ...
