[PATCH] efi/esrt: cleanup bad memory map log messages

[PATCH] efi/esrt: cleanup bad memory map log messages

[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.

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.

Signed-off-by: Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
---
 drivers/firmware/efi/efi.c  | 1 -
 drivers/firmware/efi/esrt.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 9291480..8c3ebcb 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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;
 }
 
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 1491407..d81560f 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -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

Re: [PATCH] efi/esrt: cleanup bad memory map log messages

[Ard Biesheuvel]

On 7 February 2017 at 19:08, Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org> wrote:
> 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.
>
> Signed-off-by: Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

Acked-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> ---
>  drivers/firmware/efi/efi.c  | 1 -
>  drivers/firmware/efi/esrt.c | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 9291480..8c3ebcb 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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;
>  }
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 1491407..d81560f 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -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
>

Re: [PATCH] efi/esrt: cleanup bad memory map log messages

[Peter Jones]

On Tue, Feb 07, 2017 at 01:08:23PM -0600, Daniel Drake wrote:
> 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.

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.

> 
> Signed-off-by: Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/firmware/efi/efi.c  | 1 -
>  drivers/firmware/efi/esrt.c | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 9291480..8c3ebcb 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -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;
>  }
>  
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 1491407..d81560f 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -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
> 

-- 
        Peter

Re: [PATCH] efi/esrt: cleanup bad memory map log messages

[Daniel Drake]

On Wed, Feb 8, 2017 at 8:22 AM, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 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"

> 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.

> 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

Re: [PATCH] efi/esrt: cleanup bad memory map log messages

[Peter Jones]

On Wed, Feb 08, 2017 at 09:42:34AM -0600, Daniel Drake wrote:
> On Wed, Feb 8, 2017 at 8:22 AM, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > 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"
> 
> > 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.

Fair enough.

> > 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.

Oh!  Then I've misunderstood your commit message.  You said:

> 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.

Acked-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

-- 
        Peter

Re: [PATCH] efi/esrt: cleanup bad memory map log messages

[Ard Biesheuvel]

On 8 February 2017 at 15:56, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, Feb 08, 2017 at 09:42:34AM -0600, Daniel Drake wrote:
>> On Wed, Feb 8, 2017 at 8:22 AM, Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> > 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"
>>
>> > 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.
>
> Fair enough.
>
>> > 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.
>
> Oh!  Then I've misunderstood your commit message.  You said:
>
>> 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.
>
> Acked-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>

Applied with Peter's ack

Thanks,

[PATCH] efi/esrt: Cleanup bad memory map log messages

[Ard Biesheuvel]

From: Daniel Drake <drake@endlessm.com>

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.

Signed-off-by: Daniel Drake <drake@endlessm.com>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Acked-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/firmware/efi/efi.c  | 1 -
 drivers/firmware/efi/esrt.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index e7d404059b73..b372aad3b449 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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;
 }
 
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 08b026864d4e..8554d7aec31c 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -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.7.4

[PATCH] efi/esrt: Cleanup bad memory map log messages

[Ard Biesheuvel]

From: Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>

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.

Signed-off-by: Daniel Drake <drake-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
Cc: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
Acked-by: Peter Jones <pjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/firmware/efi/efi.c  | 1 -
 drivers/firmware/efi/esrt.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index e7d404059b73..b372aad3b449 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -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;
 }
 
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 08b026864d4e..8554d7aec31c 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -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.7.4