efi/esrt: cleanup bad memory map log messages
PATCH 1/1 efi/esrt: cleanup bad memory map log messages
The Intel Compute Stick STCK1A8LFC and Weibu F3C platforms both log 2 error messages during boot:
efi: requested map not found.
esrt: ESRT header is not in the memory map.
Searching the web, this seems to affect many other platforms too. Since these messages are logged as errors, they appear on-screen during the boot process even when using the "quiet" boot parameter used by distros.
Demote the ESRT error to a warning so that it does not appear on-screen, and delete the error logging from efi_mem_desc_lookup; both callsites of that function log more specific messages upon failure.
Out of curiosity I looked closer at the Weibu F3C. There is no entry in the UEFI-provided memory map which corresponds to the ESRT pointer, but hacking the code to map it anyway, the ESRT does appear to be valid with 2 entries.
--- drivers/firmware/efi/efi.c | 1 - drivers/firmware/efi/esrt.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)
drivers/firmware/efi/efi.c +0 -1
@@ -389,7 +389,6 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)return 0 ; } } -pr_err_once ("requested map not found.\n" );return -ENOENT ; }
drivers/firmware/efi/esrt.c +1 -1
@@ -254,7 +254,7 @@ void __init efi_esrt_init(void) rc =efi_mem_desc_lookup (efi.esrt, &md);if (rc <0 ) { -pr_err ("ESRT header is not in the memory map.\n" ); +pr_warn ("ESRT header is not in the memory map.\n" );return ; }
Signature · 1 line
2.9.3
A message that is not in the archive (sent off-list or not yet received): 20170207190823.10223-1-drake-6IF/jdPJHihWk0Htik3J/[email protected]
Ard Biesheuvel
Acked-byQuoted 15 lines · Daniel Drake
The Intel Compute Stick STCK1A8LFC and Weibu F3C platforms both log 2 error messages during boot:
efi: requested map not found.
esrt: ESRT header is not in the memory map.Searching the web, this seems to affect many other platforms too. Since these messages are logged as errors, they appear on-screen during the boot process even when using the "quiet" boot parameter used by distros.
Demote the ESRT error to a warning so that it does not appear on-screen, and delete the error logging from
efi_mem_desc_lookup; both callsites of that function log more specific messages upon failure.
Quoted 6 lines
Out of curiosity I looked closer at the Weibu F3C. There is no entry in the UEFI-provided memory map which corresponds to the ESRT pointer, but hacking the code to map it anyway, the ESRT does appear to be valid with 2 entries.
Quoted 33 lines
--- drivers/firmware/efi/efi.c | 1 - drivers/firmware/efi/esrt.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)drivers/firmware/efi/efi.c +0
-1@@ -389,7 +389,6 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)return 0 ; } } -pr_err_once ("requested map not found.\n" );return -ENOENT ; }drivers/firmware/efi/esrt.c +1
-2@@ -254,7 +254,7 @@ void __init efi_esrt_init(void) rc =efi_mem_desc_lookup (efi.esrt, &md);if (rc <0 ) { -pr_err ("ESRT header is not in the memory map.\n" ); +pr_warn ("ESRT header is not in the memory map.\n" );return ; } --2.9.3
Quoted 11 lines · Daniel Drake
The Intel Compute Stick STCK1A8LFC and Weibu F3C platforms both log 2 error messages during boot:
efi: requested map not found.
esrt: ESRT header is not in the memory map.Searching the web, this seems to affect many other platforms too. Since these messages are logged as errors, they appear on-screen during the boot process even when using the "quiet" boot parameter used by distros.
Quoted 8 lines
Demote the ESRT error to a warning so that it does not appear on-screen, and delete the error logging from
efi_mem_desc_lookup; both callsites of that function log more specific messages upon failure.Out of curiosity I looked closer at the Weibu F3C. There is no entry in the UEFI-provided memory map which corresponds to the ESRT pointer, but hacking the code to map it anyway, the ESRT does appear to be valid with 2 entries.
The machine you've got does seem to be presenting a valid (if poorly considered) table, but I don't quite think this is the right approach. The first hunk of your patch is fine, but the test on the result of efi_mem_desc_lookup() is more or less our check for "might reading this table reboot the system or worse". You're demoting it to a warning while still /treating/ it as an error, so at least it remains safe, but the when we get to that point, they've (plausibly) given us data that could cause horrible behavior, and that is a thing worth logging an error about.
It would be better to /check/ if it straddles multiple contiguous segments which both have reasonable access modes and print an error if there are gaps. I'm still in favor of printing a warning to the log if it straddles them and there /aren't/ gaps, because we're *going* to see other weird bugs from any code base that decides it's stitching config tables together from separate allocations. It's not a fundamentally reasonable thing to do, and as a lens it shows us some code with a really worrisome internal structure.
Quoted 35 lines
--- drivers/firmware/efi/efi.c | 1 - drivers/firmware/efi/esrt.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)drivers/firmware/efi/efi.c +0
-1@@ -389,7 +389,6 @@ int __init efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)return 0 ; } } -pr_err_once ("requested map not found.\n" );return -ENOENT ; }drivers/firmware/efi/esrt.c +1
-1@@ -254,7 +254,7 @@ void __init efi_esrt_init(void) rc =efi_mem_desc_lookup (efi.esrt, &md);if (rc <0 ) { -pr_err ("ESRT header is not in the memory map.\n" ); +pr_warn ("ESRT header is not in the memory map.\n" );return ; }Signature · 1 line
2.9.3
Signature · 1 line
Peter
A message that is not in the archive (sent off-list or not yet received): 20170208142201.bjwafwxykj3i2icu-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Quoted 2 lines · Peter Jones
The machine you've got does seem to be presenting a valid (if poorly considered) table, but I don't quite think this is the right approach.
I don't think it is fully valid. Section 22.3 of the UEFI spec says:
"The ESRT shall be stored in memory of type EfiBootServicesData"
Quoted 7 lines
The first hunk of your patch is fine, but the test on the result of
efi_mem_desc_lookup()is more or less our check for "might reading this table reboot the system or worse". You're demoting it to a warning while still /treating/ it as an error, so at least it remains safe, but the when we get to that point, they've (plausibly) given us data that could cause horrible behavior, and that is a thing worth logging an error about.
Although from the user's perspective (thinking about graphical experience) it's a bit unfortunate to have early boot code like this print a cryptic error on-screen, when the system is otherwise perfectly usable and they might not even be interested in the ESRT's functionality.
Quoted 8 lines
It would be better to /check/ if it straddles multiple contiguous segments which both have reasonable access modes and print an error if there are gaps. I'm still in favor of printing a warning to the log if it straddles them and there /aren't/ gaps, because we're *going* to see other weird bugs from any code base that decides it's stitching config tables together from separate allocations. It's not a fundamentally reasonable thing to do, and as a lens it shows us some code with a really worrisome internal structure.
If I'm following your suggestion correctly I think it would not affect the system I have here, where the ESRT sits entirely within a gap in the memory map, without touching the allocations on either side.
Here is a dump of the memory map. type and pages are shown as decimal, the other fields hex. The ESRT pointer is 0x7b141000.
attr=f type=7 addr=0 pages=1
attr=f type=2 addr=1000 pages=1
attr=f type=7 addr=2000 pages=141
attr=f type=10 addr=8f000 pages=1
attr=f type=7 addr=90000 pages=14
attr=f type=0 addr=9e000 pages=2
attr=f type=7 addr=100000 pages=3840
attr=f type=2 addr=1000000 pages=5817
attr=f type=7 addr=26b9000 pages=121159
attr=f type=0 addr=20000000 pages=512
attr=f type=7 addr=20200000 pages=124209 attr=f type=2 addr=3e731000 pages=6351
attr=f type=7 addr=40000000 pages=103375 attr=f type=2 addr=593cf000 pages=122940 attr=f type=7 addr=7740b000 pages=6
attr=f type=2 addr=77411000 pages=3110
attr=f type=1 addr=78037000 pages=259
attr=f type=4 addr=7813a000 pages=7910
attr=f type=7 addr=7a020000 pages=3806
attr=f type=3 addr=7aefe000 pages=533
attr=f type=0 addr=7b113000 pages=48
attr=f type=7 addr=7b143000 pages=261
attr=f type=10 addr=7b248000 pages=1287 attr=800000000000000f type=6 addr=7b74f000 pages=696 attr=800000000000000f type=5 addr=7ba07000 pages=106 attr=f type=7 addr=7ba71000 pages=1
attr=f type=4 addr=7ba72000 pages=4 attr=800000000000000f type=6 addr=7ba76000 pages=1 attr=f type=4 addr=7ba77000 pages=3 attr=800000000000000f type=6 addr=7ba7a000 pages=1 attr=f type=4 addr=7ba7b000 pages=1413 attr=8000000000000001 type=12 addr=e0000000 pages=16384 attr=8000000000000001 type=11 addr=fea00000 pages=256 attr=8000000000000001 type=11 addr=fec00000 pages=1 attr=8000000000000001 type=11 addr=fed01000 pages=1 attr=8000000000000001 type=11 addr=fed03000 pages=1 attr=8000000000000001 type=11 addr=fed06000 pages=1 attr=8000000000000001 type=11 addr=fed08000 pages=2 attr=8000000000000001 type=11 addr=fed1c000 pages=1 attr=8000000000000001 type=11 addr=fed80000 pages=64 attr=8000000000000001 type=11 addr=fee00000 pages=1 attr=8000000000000001 type=11 addr=ffc00000 pages=1024
Thanks
Daniel
A message that is not in the archive (sent off-list or not yet received): CAD8Lp46MUW95vWVMayYYAwOyjOOze-ABc7Qj+RvtrbGVf46wtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Peter Jones
Acked-byQuoted 15 lines · Daniel Drake
Quoted 2 lines · Peter Jones
The machine you've got does seem to be presenting a valid (if poorly considered) table, but I don't quite think this is the right approach.
I don't think it is fully valid. Section 22.3 of the UEFI spec says:
"The ESRT shall be stored in memory of type EfiBootServicesData"Quoted 7 lines
The first hunk of your patch is fine, but the test on the result of
efi_mem_desc_lookup()is more or less our check for "might reading this table reboot the system or worse". You're demoting it to a warning while still /treating/ it as an error, so at least it remains safe, but the when we get to that point, they've (plausibly) given us data that could cause horrible behavior, and that is a thing worth logging an error about.
Quoted 5 lines
Although from the user's perspective (thinking about graphical experience) it's a bit unfortunate to have early boot code like this print a cryptic error on-screen, when the system is otherwise perfectly usable and they might not even be interested in the ESRT's functionality.
Fair enough.
Quoted 12 lines
Quoted 8 lines
It would be better to /check/ if it straddles multiple contiguous segments which both have reasonable access modes and print an error if there are gaps. I'm still in favor of printing a warning to the log if it straddles them and there /aren't/ gaps, because we're *going* to see other weird bugs from any code base that decides it's stitching config tables together from separate allocations. It's not a fundamentally reasonable thing to do, and as a lens it shows us some code with a really worrisome internal structure.
If I'm following your suggestion correctly I think it would not affect the system I have here, where the ESRT sits entirely within a gap in the memory map, without touching the allocations on either side.
Quoted 4 lines · Oh! Then I've misunderstood your commit message. You
Out of curiosity I looked closer at the Weibu F3C. There is no entry in the UEFI-provided memory map which corresponds to the ESRT pointer, but hacking the code to map it anyway, the ESRT does appear to be valid with 2 entries.
And I read "valid with 2 entries" to mean it straddles across two memory map entries, but what you're saying is it has two entries in the ESRT.
So the machine is buggy, but you're right.
Signature · 1 line
Peter
A message that is not in the archive (sent off-list or not yet received): 20170208155646.rxtctllfvqywrdor-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Quoted 44 lines · Peter Jones
Quoted 15 lines · Daniel Drake
Quoted 2 lines · Peter Jones
The machine you've got does seem to be presenting a valid (if poorly considered) table, but I don't quite think this is the right approach.
I don't think it is fully valid. Section 22.3 of the UEFI spec says:
"The ESRT shall be stored in memory of type EfiBootServicesData"Quoted 7 lines
The first hunk of your patch is fine, but the test on the result of
efi_mem_desc_lookup()is more or less our check for "might reading this table reboot the system or worse". You're demoting it to a warning while still /treating/ it as an error, so at least it remains safe, but the when we get to that point, they've (plausibly) given us data that could cause horrible behavior, and that is a thing worth logging an error about.Quoted 5 lines
Although from the user's perspective (thinking about graphical experience) it's a bit unfortunate to have early boot code like this print a cryptic error on-screen, when the system is otherwise perfectly usable and they might not even be interested in the ESRT's functionality.
Fair enough.
Quoted 12 lines
Quoted 8 lines
It would be better to /check/ if it straddles multiple contiguous segments which both have reasonable access modes and print an error if there are gaps. I'm still in favor of printing a warning to the log if it straddles them and there /aren't/ gaps, because we're *going* to see other weird bugs from any code base that decides it's stitching config tables together from separate allocations. It's not a fundamentally reasonable thing to do, and as a lens it shows us some code with a really worrisome internal structure.
If I'm following your suggestion correctly I think it would not affect the system I have here, where the ESRT sits entirely within a gap in the memory map, without touching the allocations on either side.
Quoted 4 lines · Oh! Then I've misunderstood your commit message. You
Out of curiosity I looked closer at the Weibu F3C. There is no entry in the UEFI-provided memory map which corresponds to the ESRT pointer, but hacking the code to map it anyway, the ESRT does appear to be valid with 2 entries.
Quoted 7 lines
And I read "valid with 2 entries" to mean it straddles across two memory map entries, but what you're saying is it has two entries in the ESRT.
So the machine is buggy, but you're right.
Acked-by Peter Jones <[email protected]>
Applied with Peter's ack
Thanks,