efi: memattr: don't bail on zero VA if it equals the region's PA

The EFI memory attributes code cross-references the EFI memory map with
the more granular EFI memory attributes table to ensure that they are in
sync before applying the strict permissions to the regions it describes.

Since we always install virtual mappings for the EFI runtime regions to
which these strict permissions apply, we currently perform a sanity check
on the EFI memory descriptor, and ensure that the EFI_MEMORY_RUNTIME bit
is set, and that the virtual address has been assigned.

However, in cases where a runtime region exists at physical address 0x0,
and the virtual mapping equals the physical mapping, e.g., when running
in mixed mode on x86, we encounter a memory descriptor with the runtime
attribute and virtual address 0x0, and incorrectly draw the conclusion
that a runtime region exists for which no virtual mapping was installed,
and give up altogether. The consequence of this is that firmware mappings
retain their read-write-execute permissions, making the system more
vulnerable to attacks.

So let's only bail if the virtual address of 0x0 has been assigned to a
physical region that does not reside at address 0x0.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 drivers/firmware/efi/memattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

On Mon, 24 Dec 2018 at 17:22, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> The EFI memory attributes code cross-references the EFI memory map with

> the more granular EFI memory attributes table to ensure that they are in

> sync before applying the strict permissions to the regions it describes.

>

> Since we always install virtual mappings for the EFI runtime regions to

> which these strict permissions apply, we currently perform a sanity check

> on the EFI memory descriptor, and ensure that the EFI_MEMORY_RUNTIME bit

> is set, and that the virtual address has been assigned.

>

> However, in cases where a runtime region exists at physical address 0x0,

> and the virtual mapping equals the physical mapping, e.g., when running

> in mixed mode on x86, we encounter a memory descriptor with the runtime

> attribute and virtual address 0x0, and incorrectly draw the conclusion

> that a runtime region exists for which no virtual mapping was installed,

> and give up altogether. The consequence of this is that firmware mappings

> retain their read-write-execute permissions, making the system more

> vulnerable to attacks.

>

> So let's only bail if the virtual address of 0x0 has been assigned to a

> physical region that does not reside at address 0x0.

>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Fixes: 10f0d2f577053 ("efi: Implement generic support for the Memory
Attributes table")

> ---

>  drivers/firmware/efi/memattr.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c

> index 8f289dbc237c..58452fde92cc 100644

> --- a/drivers/firmware/efi/memattr.c

> +++ b/drivers/firmware/efi/memattr.c

> @@ -91,7 +91,7 @@ static bool entry_is_valid(const efi_memory_desc_t *in, efi_memory_desc_t *out)

>

>                 if (!(md->attribute & EFI_MEMORY_RUNTIME))

>                         continue;

> -               if (md->virt_addr == 0) {

> +               if (md->virt_addr == 0 && md->phys_addr != 0) {

>                         /* no virtual mapping has been installed by the stub */

>                         break;

>                 }

> --

> 2.17.1

>

> The EFI memory attributes code cross-references the EFI memory map with the

> more granular EFI memory attributes table to ensure that they are in sync before

> applying the strict permissions to the regions it describes.

> 

> Since we always install virtual mappings for the EFI runtime regions to which

> these strict permissions apply, we currently perform a sanity check on the EFI

> memory descriptor, and ensure that the EFI_MEMORY_RUNTIME bit is set, and

> that the virtual address has been assigned.

> 

> However, in cases where a runtime region exists at physical address 0x0, and the

> virtual mapping equals the physical mapping, e.g., when running in mixed mode

> on x86, we encounter a memory descriptor with the runtime attribute and

> virtual address 0x0, and incorrectly draw the conclusion that a runtime region

> exists for which no virtual mapping was installed, and give up altogether. The

> consequence of this is that firmware mappings retain their read-write-execute

> permissions, making the system more vulnerable to attacks.

> 

> So let's only bail if the virtual address of 0x0 has been assigned to a physical

> region that does not reside at address 0x0.

> 

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  drivers/firmware/efi/memattr.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/firmware/efi/memattr.c b/drivers/firmware/efi/memattr.c

> index 8f289dbc237c..58452fde92cc 100644

> --- a/drivers/firmware/efi/memattr.c

> +++ b/drivers/firmware/efi/memattr.c

> @@ -91,7 +91,7 @@ static bool entry_is_valid(const efi_memory_desc_t *in,

> efi_memory_desc_t *out)

> 

>  		if (!(md->attribute & EFI_MEMORY_RUNTIME))

>  			continue;

> -		if (md->virt_addr == 0) {

> +		if (md->virt_addr == 0 && md->phys_addr != 0) {

>  			/* no virtual mapping has been installed by the stub */

>  			break;

>  		}


Thanks for the explanation Ard.
This change looks good to me.

Presently, on x86, even when the mapping fails we don't have any scenario where in 
md->phys_addr != 0 && md->virt_addr == 0

Regards,
Sai