[PATCH 1/3] platform/x86: Check validity of EBDA pointer in mpparse.c

[PATCH 1/3] platform/x86: Check validity of EBDA pointer in mpparse.c

[Vit Kabele]

The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
In case that the memory there is not initialized and contains garbage,
it might happen that the kernel touches memory above 640K.

This may cause unwanted reads from VGA memory which may not be decoded,
or even present when running under virtualization.

This patch adds sanity check for the EBDA pointer retrieved from the memory
so that scanning EBDA does not leave the low memory.

Signed-off-by: Vit Kabele <vit@kabele.me>
Reviewed-by: Rudolf Marek <r.marek@assembler.cz>
---
 arch/x86/include/asm/bios_ebda.h |  3 +++
 arch/x86/kernel/ebda.c           |  3 ---
 arch/x86/kernel/mpparse.c        | 12 +++++++++++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/bios_ebda.h b/arch/x86/include/asm/bios_ebda.h
index 4d5a17e2febe..c3133c01d5b7 100644
--- a/arch/x86/include/asm/bios_ebda.h
+++ b/arch/x86/include/asm/bios_ebda.h
@@ -4,6 +4,9 @@
 
 #include <asm/io.h>
 
+#define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
+#define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
+
 /*
  * Returns physical address of EBDA.  Returns 0 if there is no EBDA.
  */
diff --git a/arch/x86/kernel/ebda.c b/arch/x86/kernel/ebda.c
index 38e7d597b660..86c0801fc3ce 100644
--- a/arch/x86/kernel/ebda.c
+++ b/arch/x86/kernel/ebda.c
@@ -50,9 +50,6 @@
 
 #define BIOS_RAM_SIZE_KB_PTR	0x413
 
-#define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
-#define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
-
 void __init reserve_bios_regions(void)
 {
 	unsigned int bios_start, ebda_start;
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index fed721f90116..6bba0744d32d 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -633,7 +633,17 @@ void __init default_find_smp_config(void)
 	 */
 
 	address = get_bios_ebda();
-	if (address)
+
+	/* Check that the EBDA address is sane and the get_bios_ebda() did not
+	 * return just garbage from memory.
+	 * The upper bound is considered valid if it points below 1K before
+	 * end of the lower memory (i.e. 639K). The EBDA can be smaller
+	 * than 1K in which case the pointer will point above 639K but that
+	 * case is handled in step 2) above, and we don't need to adjust scan
+	 * size to not bump into the memory above 640K.
+	 */
+	if (address >= BIOS_START_MIN &&
+	    address < 639 * 0x400)
 		smp_scan_config(address, 0x400);
 }
 
-- 
2.30.2

[Devel] [PATCH 1/3] platform/x86: Check validity of EBDA pointer in mpparse.c

[Vit Kabele]

[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]

The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
In case that the memory there is not initialized and contains garbage,
it might happen that the kernel touches memory above 640K.

This may cause unwanted reads from VGA memory which may not be decoded,
or even present when running under virtualization.

This patch adds sanity check for the EBDA pointer retrieved from the memory
so that scanning EBDA does not leave the low memory.

Signed-off-by: Vit Kabele <vit(a)kabele.me>
Reviewed-by: Rudolf Marek <r.marek(a)assembler.cz>
---
 arch/x86/include/asm/bios_ebda.h |  3 +++
 arch/x86/kernel/ebda.c           |  3 ---
 arch/x86/kernel/mpparse.c        | 12 +++++++++++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/bios_ebda.h b/arch/x86/include/asm/bios_ebda.h
index 4d5a17e2febe..c3133c01d5b7 100644
--- a/arch/x86/include/asm/bios_ebda.h
+++ b/arch/x86/include/asm/bios_ebda.h
@@ -4,6 +4,9 @@
 
 #include <asm/io.h>
 
+#define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
+#define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
+
 /*
  * Returns physical address of EBDA.  Returns 0 if there is no EBDA.
  */
diff --git a/arch/x86/kernel/ebda.c b/arch/x86/kernel/ebda.c
index 38e7d597b660..86c0801fc3ce 100644
--- a/arch/x86/kernel/ebda.c
+++ b/arch/x86/kernel/ebda.c
@@ -50,9 +50,6 @@
 
 #define BIOS_RAM_SIZE_KB_PTR	0x413
 
-#define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
-#define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
-
 void __init reserve_bios_regions(void)
 {
 	unsigned int bios_start, ebda_start;
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index fed721f90116..6bba0744d32d 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -633,7 +633,17 @@ void __init default_find_smp_config(void)
 	 */
 
 	address = get_bios_ebda();
-	if (address)
+
+	/* Check that the EBDA address is sane and the get_bios_ebda() did not
+	 * return just garbage from memory.
+	 * The upper bound is considered valid if it points below 1K before
+	 * end of the lower memory (i.e. 639K). The EBDA can be smaller
+	 * than 1K in which case the pointer will point above 639K but that
+	 * case is handled in step 2) above, and we don't need to adjust scan
+	 * size to not bump into the memory above 640K.
+	 */
+	if (address >= BIOS_START_MIN &&
+	    address < 639 * 0x400)
 		smp_scan_config(address, 0x400);
 }
 
-- 
2.30.2

[PATCH 2/3] acpica: Check that the EBDA pointer is in valid range

[Vit Kabele]

If the memory at 0x40e is uninitialized, the retrieved physical_memory
address of EBDA may be beyond the low memory (i.e. above 640K).

If so, the kernel may unintentionally access the VGA memory, that
might not be decoded or even present in case of virtualization.

Signed-off-by: Vit Kabele <vit@kabele.me>
Reviewed-by: Rudolf Marek <r.marek@assembler.cz>
---
 drivers/acpi/acpica/tbxfroot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
index 9fec3df6c3ba..67b7df1c0520 100644
--- a/drivers/acpi/acpica/tbxfroot.c
+++ b/drivers/acpi/acpica/tbxfroot.c
@@ -138,8 +138,11 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
 	acpi_os_unmap_memory(table_ptr, ACPI_EBDA_PTR_LENGTH);
 
 	/* EBDA present? */
-
-	if (physical_address > 0x400) {
+	/* Check that the EBDA pointer from 0x40e is sane and does not point
+	 * above valid low memory
+	 */
+	if (physical_address > 0x400 &&
+	    physical_address < 0xA0000) {
 		/*
 		 * 1b) Search EBDA paragraphs (EBDA is required to be a
 		 *     minimum of 1K length)
-- 
2.30.2

[Devel] [PATCH 2/3] acpica: Check that the EBDA pointer is in valid range

[Vit Kabele]

[-- Attachment #1: Type: text/plain, Size: 1165 bytes --]

If the memory at 0x40e is uninitialized, the retrieved physical_memory
address of EBDA may be beyond the low memory (i.e. above 640K).

If so, the kernel may unintentionally access the VGA memory, that
might not be decoded or even present in case of virtualization.

Signed-off-by: Vit Kabele <vit(a)kabele.me>
Reviewed-by: Rudolf Marek <r.marek(a)assembler.cz>
---
 drivers/acpi/acpica/tbxfroot.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
index 9fec3df6c3ba..67b7df1c0520 100644
--- a/drivers/acpi/acpica/tbxfroot.c
+++ b/drivers/acpi/acpica/tbxfroot.c
@@ -138,8 +138,11 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
 	acpi_os_unmap_memory(table_ptr, ACPI_EBDA_PTR_LENGTH);
 
 	/* EBDA present? */
-
-	if (physical_address > 0x400) {
+	/* Check that the EBDA pointer from 0x40e is sane and does not point
+	 * above valid low memory
+	 */
+	if (physical_address > 0x400 &&
+	    physical_address < 0xA0000) {
 		/*
 		 * 1b) Search EBDA paragraphs (EBDA is required to be a
 		 *     minimum of 1K length)
-- 
2.30.2

[PATCH 3/3] acpica: Do not touch VGA memory when EBDA < 1KiB

[Vit Kabele]

The ACPICA code assumes that EBDA region must be at least 1KiB in size.
Because this is not guaranteed, it might happen that while scanning the
memory for RSDP pointer, the kernel touches memory above 640KiB.

This is unwanted as the VGA memory range may not be decoded or
even present when running under virtualization.

Signed-off-by: Vit Kabele <vit@kabele.me>
Reviewed-by: Rudolf Marek <r.marek@assembler.cz>
---
 drivers/acpi/acpica/tbxfroot.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
index 67b7df1c0520..b1f4a91044d9 100644
--- a/drivers/acpi/acpica/tbxfroot.c
+++ b/drivers/acpi/acpica/tbxfroot.c
@@ -114,6 +114,7 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
 	u8 *table_ptr;
 	u8 *mem_rover;
 	u32 physical_address;
+	u32 ebda_window_size;
 
 	ACPI_FUNCTION_TRACE(acpi_find_root_pointer);
 
@@ -143,25 +144,32 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
 	 */
 	if (physical_address > 0x400 &&
 	    physical_address < 0xA0000) {
+		/* Calculate the scan window size
+		 * The EBDA is not guaranteed to be larger than a KiB
+		 * and in case that it is smaller the scanning function would
+		 * leave the low memory and continue to the VGA range.
+		 */
+		ebda_window_size = ACPI_MIN(ACPI_EBDA_WINDOW_SIZE,
+					    0xA0000 - physical_address);
+
 		/*
-		 * 1b) Search EBDA paragraphs (EBDA is required to be a
-		 *     minimum of 1K length)
+		 * 1b) Search EBDA paragraphs
 		 */
 		table_ptr = acpi_os_map_memory((acpi_physical_address)
 					       physical_address,
-					       ACPI_EBDA_WINDOW_SIZE);
+					       ebda_window_size);
 		if (!table_ptr) {
 			ACPI_ERROR((AE_INFO,
 				    "Could not map memory at 0x%8.8X for length %u",
-				    physical_address, ACPI_EBDA_WINDOW_SIZE));
+				    physical_address, ebda_window_size));
 
 			return_ACPI_STATUS(AE_NO_MEMORY);
 		}
 
 		mem_rover =
 		    acpi_tb_scan_memory_for_rsdp(table_ptr,
-						 ACPI_EBDA_WINDOW_SIZE);
-		acpi_os_unmap_memory(table_ptr, ACPI_EBDA_WINDOW_SIZE);
+						 ebda_window_size);
+		acpi_os_unmap_memory(table_ptr, ebda_window_size);
 
 		if (mem_rover) {
 
-- 
2.30.2

[Devel] [PATCH 3/3] acpica: Do not touch VGA memory when EBDA < 1KiB

[Vit Kabele]

[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]

The ACPICA code assumes that EBDA region must be at least 1KiB in size.
Because this is not guaranteed, it might happen that while scanning the
memory for RSDP pointer, the kernel touches memory above 640KiB.

This is unwanted as the VGA memory range may not be decoded or
even present when running under virtualization.

Signed-off-by: Vit Kabele <vit(a)kabele.me>
Reviewed-by: Rudolf Marek <r.marek(a)assembler.cz>
---
 drivers/acpi/acpica/tbxfroot.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
index 67b7df1c0520..b1f4a91044d9 100644
--- a/drivers/acpi/acpica/tbxfroot.c
+++ b/drivers/acpi/acpica/tbxfroot.c
@@ -114,6 +114,7 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
 	u8 *table_ptr;
 	u8 *mem_rover;
 	u32 physical_address;
+	u32 ebda_window_size;
 
 	ACPI_FUNCTION_TRACE(acpi_find_root_pointer);
 
@@ -143,25 +144,32 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
 	 */
 	if (physical_address > 0x400 &&
 	    physical_address < 0xA0000) {
+		/* Calculate the scan window size
+		 * The EBDA is not guaranteed to be larger than a KiB
+		 * and in case that it is smaller the scanning function would
+		 * leave the low memory and continue to the VGA range.
+		 */
+		ebda_window_size = ACPI_MIN(ACPI_EBDA_WINDOW_SIZE,
+					    0xA0000 - physical_address);
+
 		/*
-		 * 1b) Search EBDA paragraphs (EBDA is required to be a
-		 *     minimum of 1K length)
+		 * 1b) Search EBDA paragraphs
 		 */
 		table_ptr = acpi_os_map_memory((acpi_physical_address)
 					       physical_address,
-					       ACPI_EBDA_WINDOW_SIZE);
+					       ebda_window_size);
 		if (!table_ptr) {
 			ACPI_ERROR((AE_INFO,
 				    "Could not map memory at 0x%8.8X for length %u",
-				    physical_address, ACPI_EBDA_WINDOW_SIZE));
+				    physical_address, ebda_window_size));
 
 			return_ACPI_STATUS(AE_NO_MEMORY);
 		}
 
 		mem_rover =
 		    acpi_tb_scan_memory_for_rsdp(table_ptr,
-						 ACPI_EBDA_WINDOW_SIZE);
-		acpi_os_unmap_memory(table_ptr, ACPI_EBDA_WINDOW_SIZE);
+						 ebda_window_size);
+		acpi_os_unmap_memory(table_ptr, ebda_window_size);
 
 		if (mem_rover) {
 
-- 
2.30.2

Re: [PATCH 1/3] platform/x86: Check validity of EBDA pointer in mpparse.c

[Rafael J. Wysocki]

First off, this is not platform/x86, but arch/x86.

On Thu, Mar 17, 2022 at 3:12 PM Vit Kabele <vit@kabele.me> wrote:
>
> The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
> In case that the memory there is not initialized and contains garbage,
> it might happen that the kernel touches memory above 640K.
>
> This may cause unwanted reads from VGA memory which may not be decoded,
> or even present when running under virtualization.
>
> This patch adds sanity check for the EBDA pointer retrieved from the memory
> so that scanning EBDA does not leave the low memory.
>
> Signed-off-by: Vit Kabele <vit@kabele.me>
> Reviewed-by: Rudolf Marek <r.marek@assembler.cz>
> ---
>  arch/x86/include/asm/bios_ebda.h |  3 +++
>  arch/x86/kernel/ebda.c           |  3 ---
>  arch/x86/kernel/mpparse.c        | 12 +++++++++++-
>  3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/bios_ebda.h b/arch/x86/include/asm/bios_ebda.h
> index 4d5a17e2febe..c3133c01d5b7 100644
> --- a/arch/x86/include/asm/bios_ebda.h
> +++ b/arch/x86/include/asm/bios_ebda.h
> @@ -4,6 +4,9 @@
>
>  #include <asm/io.h>
>
> +#define BIOS_START_MIN         0x20000U        /* 128K, less than this is insane */
> +#define BIOS_START_MAX         0x9f000U        /* 640K, absolute maximum */
> +
>  /*
>   * Returns physical address of EBDA.  Returns 0 if there is no EBDA.
>   */
> diff --git a/arch/x86/kernel/ebda.c b/arch/x86/kernel/ebda.c
> index 38e7d597b660..86c0801fc3ce 100644
> --- a/arch/x86/kernel/ebda.c
> +++ b/arch/x86/kernel/ebda.c
> @@ -50,9 +50,6 @@
>
>  #define BIOS_RAM_SIZE_KB_PTR   0x413
>
> -#define BIOS_START_MIN         0x20000U        /* 128K, less than this is insane */
> -#define BIOS_START_MAX         0x9f000U        /* 640K, absolute maximum */
> -
>  void __init reserve_bios_regions(void)
>  {
>         unsigned int bios_start, ebda_start;
> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fed721f90116..6bba0744d32d 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -633,7 +633,17 @@ void __init default_find_smp_config(void)
>          */
>
>         address = get_bios_ebda();
> -       if (address)
> +
> +       /* Check that the EBDA address is sane and the get_bios_ebda() did not

Comment format not adhering to coding-style.

> +        * return just garbage from memory.
> +        * The upper bound is considered valid if it points below 1K before
> +        * end of the lower memory (i.e. 639K). The EBDA can be smaller
> +        * than 1K in which case the pointer will point above 639K but that
> +        * case is handled in step 2) above, and we don't need to adjust scan
> +        * size to not bump into the memory above 640K.
> +        */
> +       if (address >= BIOS_START_MIN &&
> +           address < 639 * 0x400)

This line doesn't need to be broken and maybe define a symbol for the
upper bound limit.

And if the 0x400 simply means 1KiB, it would be less confusing to use
a decimal number IMO.

>                 smp_scan_config(address, 0x400);
>  }
>
> --

[Devel] Re: [PATCH 1/3] platform/x86: Check validity of EBDA pointer in mpparse.c

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 3186 bytes --]

First off, this is not platform/x86, but arch/x86.

On Thu, Mar 17, 2022 at 3:12 PM Vit Kabele <vit(a)kabele.me> wrote:
>
> The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
> In case that the memory there is not initialized and contains garbage,
> it might happen that the kernel touches memory above 640K.
>
> This may cause unwanted reads from VGA memory which may not be decoded,
> or even present when running under virtualization.
>
> This patch adds sanity check for the EBDA pointer retrieved from the memory
> so that scanning EBDA does not leave the low memory.
>
> Signed-off-by: Vit Kabele <vit(a)kabele.me>
> Reviewed-by: Rudolf Marek <r.marek(a)assembler.cz>
> ---
>  arch/x86/include/asm/bios_ebda.h |  3 +++
>  arch/x86/kernel/ebda.c           |  3 ---
>  arch/x86/kernel/mpparse.c        | 12 +++++++++++-
>  3 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/bios_ebda.h b/arch/x86/include/asm/bios_ebda.h
> index 4d5a17e2febe..c3133c01d5b7 100644
> --- a/arch/x86/include/asm/bios_ebda.h
> +++ b/arch/x86/include/asm/bios_ebda.h
> @@ -4,6 +4,9 @@
>
>  #include <asm/io.h>
>
> +#define BIOS_START_MIN         0x20000U        /* 128K, less than this is insane */
> +#define BIOS_START_MAX         0x9f000U        /* 640K, absolute maximum */
> +
>  /*
>   * Returns physical address of EBDA.  Returns 0 if there is no EBDA.
>   */
> diff --git a/arch/x86/kernel/ebda.c b/arch/x86/kernel/ebda.c
> index 38e7d597b660..86c0801fc3ce 100644
> --- a/arch/x86/kernel/ebda.c
> +++ b/arch/x86/kernel/ebda.c
> @@ -50,9 +50,6 @@
>
>  #define BIOS_RAM_SIZE_KB_PTR   0x413
>
> -#define BIOS_START_MIN         0x20000U        /* 128K, less than this is insane */
> -#define BIOS_START_MAX         0x9f000U        /* 640K, absolute maximum */
> -
>  void __init reserve_bios_regions(void)
>  {
>         unsigned int bios_start, ebda_start;
> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fed721f90116..6bba0744d32d 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -633,7 +633,17 @@ void __init default_find_smp_config(void)
>          */
>
>         address = get_bios_ebda();
> -       if (address)
> +
> +       /* Check that the EBDA address is sane and the get_bios_ebda() did not

Comment format not adhering to coding-style.

> +        * return just garbage from memory.
> +        * The upper bound is considered valid if it points below 1K before
> +        * end of the lower memory (i.e. 639K). The EBDA can be smaller
> +        * than 1K in which case the pointer will point above 639K but that
> +        * case is handled in step 2) above, and we don't need to adjust scan
> +        * size to not bump into the memory above 640K.
> +        */
> +       if (address >= BIOS_START_MIN &&
> +           address < 639 * 0x400)

This line doesn't need to be broken and maybe define a symbol for the
upper bound limit.

And if the 0x400 simply means 1KiB, it would be less confusing to use
a decimal number IMO.

>                 smp_scan_config(address, 0x400);
>  }
>
> --

Re: [PATCH 2/3] acpica: Check that the EBDA pointer is in valid range

[Rafael J. Wysocki]

On Thu, Mar 17, 2022 at 3:12 PM Vit Kabele <vit@kabele.me> wrote:
>
> If the memory at 0x40e is uninitialized, the retrieved physical_memory
> address of EBDA may be beyond the low memory (i.e. above 640K).
>
> If so, the kernel may unintentionally access the VGA memory, that
> might not be decoded or even present in case of virtualization.
>
> Signed-off-by: Vit Kabele <vit@kabele.me>
> Reviewed-by: Rudolf Marek <r.marek@assembler.cz>
> ---
>  drivers/acpi/acpica/tbxfroot.c | 7 +++++--

Changes in the ACPICA code need to be submitted to the upstream ACPICA
project via https://github.com/acpica/acpica and they are pulled from
there into the kernel.

Of course, this applies to the next patch in the series too.

>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
> index 9fec3df6c3ba..67b7df1c0520 100644
> --- a/drivers/acpi/acpica/tbxfroot.c
> +++ b/drivers/acpi/acpica/tbxfroot.c
> @@ -138,8 +138,11 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
>         acpi_os_unmap_memory(table_ptr, ACPI_EBDA_PTR_LENGTH);
>
>         /* EBDA present? */
> -
> -       if (physical_address > 0x400) {
> +       /* Check that the EBDA pointer from 0x40e is sane and does not point
> +        * above valid low memory
> +        */
> +       if (physical_address > 0x400 &&
> +           physical_address < 0xA0000) {
>                 /*
>                  * 1b) Search EBDA paragraphs (EBDA is required to be a
>                  *     minimum of 1K length)
> --
> 2.30.2
>

[Devel] Re: [PATCH 2/3] acpica: Check that the EBDA pointer is in valid range

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

On Thu, Mar 17, 2022 at 3:12 PM Vit Kabele <vit(a)kabele.me> wrote:
>
> If the memory at 0x40e is uninitialized, the retrieved physical_memory
> address of EBDA may be beyond the low memory (i.e. above 640K).
>
> If so, the kernel may unintentionally access the VGA memory, that
> might not be decoded or even present in case of virtualization.
>
> Signed-off-by: Vit Kabele <vit(a)kabele.me>
> Reviewed-by: Rudolf Marek <r.marek(a)assembler.cz>
> ---
>  drivers/acpi/acpica/tbxfroot.c | 7 +++++--

Changes in the ACPICA code need to be submitted to the upstream ACPICA
project via https://github.com/acpica/acpica and they are pulled from
there into the kernel.

Of course, this applies to the next patch in the series too.

>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/acpica/tbxfroot.c b/drivers/acpi/acpica/tbxfroot.c
> index 9fec3df6c3ba..67b7df1c0520 100644
> --- a/drivers/acpi/acpica/tbxfroot.c
> +++ b/drivers/acpi/acpica/tbxfroot.c
> @@ -138,8 +138,11 @@ acpi_find_root_pointer(acpi_physical_address *table_address)
>         acpi_os_unmap_memory(table_ptr, ACPI_EBDA_PTR_LENGTH);
>
>         /* EBDA present? */
> -
> -       if (physical_address > 0x400) {
> +       /* Check that the EBDA pointer from 0x40e is sane and does not point
> +        * above valid low memory
> +        */
> +       if (physical_address > 0x400 &&
> +           physical_address < 0xA0000) {
>                 /*
>                  * 1b) Search EBDA paragraphs (EBDA is required to be a
>                  *     minimum of 1K length)
> --
> 2.30.2
>

[PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

[Vit Kabele]

The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
In case that the memory there is not initialized and contains garbage,
it might happen that the kernel touches memory above 640K.

This may cause unwanted reads from VGA memory which may not be decoded,
or even present when running under virtualization.

This patch adds sanity check for the EBDA pointer retrieved from the memory
so that scanning EBDA does not leave the low memory.

Signed-off-by: Vit Kabele <vit@kabele.me>
Reviewed-by: Rudolf Marek <r.marek@assembler.cz>
---
changes in v2:
 * Fix the comment formating
 * Change the condition. I used already defined symbol for easier
    interpretation

 arch/x86/include/asm/bios_ebda.h |  3 +++
 arch/x86/kernel/ebda.c           |  3 ---
 arch/x86/kernel/mpparse.c        | 14 ++++++++++++--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/bios_ebda.h b/arch/x86/include/asm/bios_ebda.h
index 4d5a17e2febe..c3133c01d5b7 100644
--- a/arch/x86/include/asm/bios_ebda.h
+++ b/arch/x86/include/asm/bios_ebda.h
@@ -4,6 +4,9 @@
 
 #include <asm/io.h>
 
+#define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
+#define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
+
 /*
  * Returns physical address of EBDA.  Returns 0 if there is no EBDA.
  */
diff --git a/arch/x86/kernel/ebda.c b/arch/x86/kernel/ebda.c
index 38e7d597b660..86c0801fc3ce 100644
--- a/arch/x86/kernel/ebda.c
+++ b/arch/x86/kernel/ebda.c
@@ -50,9 +50,6 @@
 
 #define BIOS_RAM_SIZE_KB_PTR	0x413
 
-#define BIOS_START_MIN		0x20000U	/* 128K, less than this is insane */
-#define BIOS_START_MAX		0x9f000U	/* 640K, absolute maximum */
-
 void __init reserve_bios_regions(void)
 {
 	unsigned int bios_start, ebda_start;
diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
index fed721f90116..9e0b4820f33b 100644
--- a/arch/x86/kernel/mpparse.c
+++ b/arch/x86/kernel/mpparse.c
@@ -633,8 +633,18 @@ void __init default_find_smp_config(void)
 	 */
 
 	address = get_bios_ebda();
-	if (address)
-		smp_scan_config(address, 0x400);
+
+	/*
+	 * Check that the EBDA address is sane and the get_bios_ebda() did not
+	 * return just garbage from memory.
+	 * The upper bound is considered valid if it points below 1K before
+	 * end of the lower memory (i.e. 639K). The EBDA can be smaller
+	 * than 1K in which case the pointer will point above 639K but that
+	 * case is handled in step 2) above, and we don't need to adjust scan
+	 * size to not bump into the memory above 640K.
+	 */
+	if (address >= BIOS_START_MIN && address < (BIOS_START_MAX - 1024))
+		smp_scan_config(address, 1024);
 }
 
 #ifdef CONFIG_X86_IO_APIC
-- 
2.30.2

Re: [PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

[Borislav Petkov]

On Fri, Apr 08, 2022 at 10:46:46AM +0200, Vit Kabele wrote:
> The pointer to EBDA area is retrieved from a word at 0x40e in BDA.
> In case that the memory there is not initialized and contains garbage,
> it might happen that the kernel touches memory above 640K.

This is where I'm missing the "why do this" at all. Grepping back in
my mbox, I see another thread from you where you say something about
"testing custom virtualization platform".

So I'd like to see why this fix is needed so feel free to elaborate in the
commit message what the situation is and why you're doing this.

> This may cause unwanted reads from VGA memory which may not be decoded,
> or even present when running under virtualization.
> 
> This patch adds sanity check for the EBDA pointer retrieved from the memory

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

IOW,

s/This patch adds/Add/

> diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> index fed721f90116..9e0b4820f33b 100644
> --- a/arch/x86/kernel/mpparse.c
> +++ b/arch/x86/kernel/mpparse.c
> @@ -633,8 +633,18 @@ void __init default_find_smp_config(void)
>  	 */
>  
>  	address = get_bios_ebda();
> -	if (address)
> -		smp_scan_config(address, 0x400);
> +
> +	/*
> +	 * Check that the EBDA address is sane and the get_bios_ebda() did not
> +	 * return just garbage from memory.
> +	 * The upper bound is considered valid if it points below 1K before
> +	 * end of the lower memory (i.e. 639K). The EBDA can be smaller
> +	 * than 1K in which case the pointer will point above 639K but that
> +	 * case is handled in step 2) above, and we don't need to adjust scan
> +	 * size to not bump into the memory above 640K.
> +	 */
> +	if (address >= BIOS_START_MIN && address < (BIOS_START_MAX - 1024))
> +		smp_scan_config(address, 1024);
>  }

I guess but looking at reserve_bios_regions(), that function is already
doing kinda the same along with being a bit more careful to figure out
bios_start so you could unify the code into a common helper and use it
at both places?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

[Vit Kabele]

On Tue, May 03, 2022 at 07:36:35PM +0200, Borislav Petkov wrote:
> > diff --git a/arch/x86/kernel/mpparse.c b/arch/x86/kernel/mpparse.c
> > index fed721f90116..9e0b4820f33b 100644
> > --- a/arch/x86/kernel/mpparse.c
> > +++ b/arch/x86/kernel/mpparse.c
> > @@ -633,8 +633,18 @@ void __init default_find_smp_config(void)
> >  	 */
> >  
> >  	address = get_bios_ebda();
> > -	if (address)
> > -		smp_scan_config(address, 0x400);
> > +
> > +	/*
> > +	 * Check that the EBDA address is sane and the get_bios_ebda() did not
> > +	 * return just garbage from memory.
> > +	 * The upper bound is considered valid if it points below 1K before
> > +	 * end of the lower memory (i.e. 639K). The EBDA can be smaller
> > +	 * than 1K in which case the pointer will point above 639K but that
> > +	 * case is handled in step 2) above, and we don't need to adjust scan
> > +	 * size to not bump into the memory above 640K.
> > +	 */
> > +	if (address >= BIOS_START_MIN && address < (BIOS_START_MAX - 1024))
> > +		smp_scan_config(address, 1024);
> >  }
> 
> I guess but looking at reserve_bios_regions(), that function is already
> doing kinda the same along with being a bit more careful to figure out
> bios_start so you could unify the code into a common helper and use it
> at both places?
I also initially thought of extracting the check to a separate method,
but imo this decreases the overall code readability. Any function
calling the get_bios_ebda() must check the returned value anyway, so
there will be always at least one if statement involved. And the valid
upper bound of the EBDA pointer is also different for these two use-cases.
(The mpparse.c usage is interested in EBDA pointer only if it ends 1KiB
before the end of low memory, while the ebda.c accepts even the values in the
last KiB below 640KiB).

I also consider it unlikely that there will be some new code using
the same bounds check, so I'd prefer to leave it inline.

-- 
Best regards,
Vit Kabele

Re: [PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

[Borislav Petkov]

On Mon, May 16, 2022 at 11:43:48AM +0200, Vit Kabele wrote:
>  And the valid upper bound of the EBDA pointer is also different
> for these two use-cases. (The mpparse.c usage is interested in EBDA
> pointer only if it ends 1KiB before the end of low memory, while the
> ebda.c accepts even the values in the last KiB below 640KiB).

And I still don't know why this difference in the upper bounds is really
relevant and why you can't simply use the code in reserve_bios_regions()
after carving it out in a helper?

The latter considers ebda_start valid when it is between BIOS_START_MIN
and bios_start, after having sanitized that bios_start to 640K if "out
of bounds".

Why can't default_find_smp_config() simply scan the last KiB below
640KiB twice for the sake of simpler code?

I.e., there needs to be a single get_bios_ebda() - the current one can
be renamed to __get_bios_ebda() - and that get_bios_ebda() should give
either a sane EBDA address or 0 if the checks don't pass. And all code
should use that.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v2] arch/x86: Check validity of EBDA pointer in mpparse.c

[Vit Kabele]

On Tue, May 17, 2022 at 09:21:57PM +0200, Borislav Petkov wrote:
> Why can't default_find_smp_config() simply scan the last KiB below
> 640KiB twice for the sake of simpler code?
The problem is not in scanning the last KiB twice. But when the
ebda start is between 639 and 640 KiB, we need to adjust the size of the
scan window like MIN(1024, 640 * 1024 - address), because we don't want
to bump into the memory above 640K.

This is obviously not a problem to do, but since we are talking about
+/- a few lines, I thought it is more readable like that.

> I.e., there needs to be a single get_bios_ebda() - the current one can
> be renamed to __get_bios_ebda() - and that get_bios_ebda() should give
> either a sane EBDA address or 0 if the checks don't pass. And all code
> should use that.
I can do that if you consider it better, but it is a little bit more
lines and some code has to be duplicated.  E.g. the
reserve_bios_regions() cares about MIN(bios_start, ebda_start), so it
needs to read the BIOS_RAM_SIZE_KB_PTR and check its sanity anyway.
Since this is basically the code that would be carved out to the new
get_bios_ebda() helper, we don't save anything.

-- 
Best regards,
Vit Kabele