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

[GIT PULL] UEFI fix for v4.11-rc

[Ard Biesheuvel]

Hi all,

At Matt's request, we are proposing this single patch as a bugfix for v4.11.
Please pull.

The following changes since commit 4495c08e84729385774601b5146d51d9e5849f81:

  Linux 4.11-rc2 (2017-03-12 14:47:08 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git tags/efi-urgent

for you to fetch changes up to 822f5845f710e57d7e2df1fd1ee00d6e19d334fe:

  efi/esrt: Cleanup bad memory map log messages (2017-03-17 18:53:12 +0000)

----------------------------------------------------------------
A single UEFI fix:
- Reduce the severity of the notice that appears when the ESRT table points
  to memory that is not covered by the memory map. It is scaring our users
  and interfering with their nice splash screens. Note that the ESRT may still
  be perfectly usable, and is currently (to my knowledge) not widely used to
  begin with.

----------------------------------------------------------------
Daniel Drake (1):
      efi/esrt: Cleanup bad memory map log messages

 drivers/firmware/efi/efi.c  | 1 -
 drivers/firmware/efi/esrt.c | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

[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