All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Fix -acpitable regression
@ 2021-12-27 19:31 Igor Mammedov
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Igor Mammedov @ 2021-12-27 19:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: dlenski, mst


Since 6.2 QEMU will assert when SLIC table is passed with                        
help of -acpitable. This series fixes the issue and adds a test                  
case for it.                                                                     
                                                                                 
                                                                                 
PS: gitlab whining:                                                              
* the issue was reported and ivestigated via shiny gitlab issue tracker,      
  the problem is that all that discussion is buried there and is not stored     
  in qemu-devel mail list. So when gitlab is gone, so will be all the history    
  and one won't have (nicely and locally stored) mail archive to search in       
  a convenient way.                                                              
  Also I'd notice the report earlier if it were forwarded to qemu-devel.         
  I wonder if there is a way to bridge issue tracker discussions to mail list?   
* another issue is that gitlab hides user's emails, with a bit of detective work 
  one can find email if the user has committed a patch via gitlab, but that doesn't
  work for every user. So I can't CC/properly credit reporter when posting formal
  patch. 

CC: dlenski@gmail.com
CC: mst@redhat.com


Igor Mammedov (4):
  acpi: fix QEMU crash when started with SLIC table
  tests: acpi: whitelist expected blobs before changing them
  tests: acpi: add SLIC table test
  tests: acpi: SLIC: update expected blobs

 hw/acpi/core.c                 |   4 ++--
 hw/i386/acpi-build.c           |   2 ++
 tests/data/acpi/q35/FACP.slic  | Bin 0 -> 244 bytes
 tests/data/acpi/q35/SLIC.slic  | Bin 0 -> 36 bytes
 tests/qtest/bios-tables-test.c |  15 +++++++++++++++
 5 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 tests/data/acpi/q35/FACP.slic
 create mode 100644 tests/data/acpi/q35/SLIC.slic

-- 
2.31.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table
  2021-12-27 19:31 [PATCH 0/4] Fix -acpitable regression Igor Mammedov
@ 2021-12-27 19:31 ` Igor Mammedov
  2021-12-27 21:26   ` Philippe Mathieu-Daudé
                     ` (3 more replies)
  2021-12-27 19:31 ` [PATCH 2/4] tests: acpi: whitelist expected blobs before changing them Igor Mammedov
                   ` (2 subsequent siblings)
  3 siblings, 4 replies; 9+ messages in thread
From: Igor Mammedov @ 2021-12-27 19:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: dlenski, mst

if QEMU is started with used provided SLIC table blob,

  -acpitable sig=SLIC,oem_id='CRASH ',oem_table_id="ME",oem_rev=00002210,asl_compiler_id="",asl_compiler_rev=00000000,data=/dev/null
it will assert with:

  hw/acpi/aml-build.c:61:build_append_padded_str: assertion failed: (len <= maxlen)

and following backtrace:

  ...
  build_append_padded_str (array=0x555556afe320, str=0x555556afdb2e "CRASH ME", maxlen=0x6, pad=0x20) at hw/acpi/aml-build.c:61
  acpi_table_begin (desc=0x7fffffffd1b0, array=0x555556afe320) at hw/acpi/aml-build.c:1727
  build_fadt (tbl=0x555556afe320, linker=0x555557ca3830, f=0x7fffffffd318, oem_id=0x555556afdb2e "CRASH ME", oem_table_id=0x555556afdb34 "ME") at hw/acpi/aml-build.c:2064
  ...

which happens due to acpi_table_begin() expecting NULL terminated
oem_id and oem_table_id strings, which is normally the case, but
in case of user provided SLIC table, oem_id points to table's blob
directly and as result oem_id became longer than expected.

Fix issue by handling oem_id consistently and make acpi_get_slic_oem()
return NULL terminated strings.

PS:
After [1] refactoring, oem_id semantics became inconsistent, where
NULL terminated string was coming from machine and old way pointer
into byte array coming from -acpitable option. That used to work
since build_header() wasn't expecting NULL terminated string and
blindly copied the 1st 6 bytes only.

However commit [2] broke that by replacing build_header() with
acpi_table_begin(), which was expecting NULL terminated string
and was checking oem_id size.

1) 602b45820 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
2)
Fixes: 4b56e1e4eb08 ("acpi: build_fadt: use acpi_table_begin()/acpi_table_end() instead of build_header()")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/786
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/core.c       | 4 ++--
 hw/i386/acpi-build.c | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 1e004d0078..3e811bf03c 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -345,8 +345,8 @@ int acpi_get_slic_oem(AcpiSlicOem *oem)
         struct acpi_table_header *hdr = (void *)(u - sizeof(hdr->_length));
 
         if (memcmp(hdr->sig, "SLIC", 4) == 0) {
-            oem->id = hdr->oem_id;
-            oem->table_id = hdr->oem_table_id;
+            oem->id = g_strndup(hdr->oem_id, 6);
+            oem->table_id = g_strndup(hdr->oem_table_id, 8);
             return 0;
         }
     }
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8383b83ee3..0234fe7588 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2723,6 +2723,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
 
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
+    g_free(slic_oem.id);
+    g_free(slic_oem.table_id);
 }
 
 static void acpi_ram_update(MemoryRegion *mr, GArray *data)
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4] tests: acpi: whitelist expected blobs before changing them
  2021-12-27 19:31 [PATCH 0/4] Fix -acpitable regression Igor Mammedov
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
@ 2021-12-27 19:31 ` Igor Mammedov
  2021-12-27 19:31 ` [PATCH 3/4] tests: acpi: add SLIC table test Igor Mammedov
  2021-12-27 19:31 ` [PATCH 4/4] tests: acpi: SLIC: update expected blobs Igor Mammedov
  3 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2021-12-27 19:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: dlenski, mst

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   2 ++
 tests/data/acpi/q35/FACP.slic               | Bin 0 -> 244 bytes
 tests/data/acpi/q35/SLIC.slic               |   0
 3 files changed, 2 insertions(+)
 create mode 100644 tests/data/acpi/q35/FACP.slic
 create mode 100644 tests/data/acpi/q35/SLIC.slic

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..49dbf8fa3e 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,3 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/q35/FACP.slic",
+"tests/data/acpi/q35/SLIC.slic",
diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
new file mode 100644
index 0000000000000000000000000000000000000000..f6a864cc863c7763f6c09d3814ad184a658fa0a0
GIT binary patch
literal 244
zcmZ>BbPo8!z`($~)5+i2BUr&HBEVSz2pEB4AU24G0Y(N+hD|^Y6El!tgNU*~X%LSC
z$X0-fGcm9T0LA|E|L2FOWMD7?GM2V5Ffej3F#P0!h{7ddihwku0+2v57svwxMxcSn
X_QAxFX+{NzJ3wNL4G8yu_%Hwf>-7!+

literal 0
HcmV?d00001

diff --git a/tests/data/acpi/q35/SLIC.slic b/tests/data/acpi/q35/SLIC.slic
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] tests: acpi: add SLIC table test
  2021-12-27 19:31 [PATCH 0/4] Fix -acpitable regression Igor Mammedov
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
  2021-12-27 19:31 ` [PATCH 2/4] tests: acpi: whitelist expected blobs before changing them Igor Mammedov
@ 2021-12-27 19:31 ` Igor Mammedov
  2021-12-27 19:31 ` [PATCH 4/4] tests: acpi: SLIC: update expected blobs Igor Mammedov
  3 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2021-12-27 19:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: dlenski, mst

When user uses '-acpitable' to add SLIC table, some ACPI
tables (FADT) will change its 'Oem ID'/'Oem Table ID' fields to
match that of SLIC. Test makes sure thati QEMU handles
those fields correctly when SLIC table is added with
'-acpitable' option.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index 9a468e29eb..e6b72d9026 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -1502,6 +1502,20 @@ static void test_acpi_virt_viot(void)
     free_test_data(&data);
 }
 
+static void test_acpi_q35_slic(void)
+{
+    test_data data = {
+        .machine = MACHINE_Q35,
+        .variant = ".slic",
+    };
+
+    test_acpi_one("-acpitable sig=SLIC,oem_id='CRASH ',oem_table_id='ME',"
+                  "oem_rev=00002210,asl_compiler_id='qemu',"
+                  "asl_compiler_rev=00000000,data=/dev/null",
+                  &data);
+    free_test_data(&data);
+}
+
 static void test_oem_fields(test_data *data)
 {
     int i;
@@ -1677,6 +1691,7 @@ int main(int argc, char *argv[])
             qtest_add_func("acpi/q35/kvm/dmar", test_acpi_q35_kvm_dmar);
         }
         qtest_add_func("acpi/q35/viot", test_acpi_q35_viot);
+        qtest_add_func("acpi/q35/slic", test_acpi_q35_slic);
     } else if (strcmp(arch, "aarch64") == 0) {
         if (has_tcg) {
             qtest_add_func("acpi/virt", test_acpi_virt_tcg);
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] tests: acpi: SLIC: update expected blobs
  2021-12-27 19:31 [PATCH 0/4] Fix -acpitable regression Igor Mammedov
                   ` (2 preceding siblings ...)
  2021-12-27 19:31 ` [PATCH 3/4] tests: acpi: add SLIC table test Igor Mammedov
@ 2021-12-27 19:31 ` Igor Mammedov
  3 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2021-12-27 19:31 UTC (permalink / raw)
  To: qemu-devel; +Cc: dlenski, mst

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/qtest/bios-tables-test-allowed-diff.h |   2 --
 tests/data/acpi/q35/FACP.slic               | Bin 244 -> 244 bytes
 tests/data/acpi/q35/SLIC.slic               | Bin 0 -> 36 bytes
 3 files changed, 2 deletions(-)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h b/tests/qtest/bios-tables-test-allowed-diff.h
index 49dbf8fa3e..dfb8523c8b 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1 @@
 /* List of comma-separated changed AML files to ignore */
-"tests/data/acpi/q35/FACP.slic",
-"tests/data/acpi/q35/SLIC.slic",
diff --git a/tests/data/acpi/q35/FACP.slic b/tests/data/acpi/q35/FACP.slic
index f6a864cc863c7763f6c09d3814ad184a658fa0a0..891fd4b784b7b6b3ea303976db7ecd5b669bc84b 100644
GIT binary patch
delta 28
jcmeyu_=Qo#&CxmF3j+fKvygL;W3Y#Uud9N>M3Dyoc<=}c

delta 28
jcmeyu_=Qo#&CxmF3j+fK^G+v!XOCb7r-%UOi6RdGgN+Fa

diff --git a/tests/data/acpi/q35/SLIC.slic b/tests/data/acpi/q35/SLIC.slic
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..fd26592e2480c5d02a018e0d855a04106661a7b5 100644
GIT binary patch
literal 36
mcmWIc@pM*UU|?YMbPjS1_E7M31#*C(gN1>iFg3Rn#0CI%)&>Cp

literal 0
HcmV?d00001

-- 
2.31.1



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
@ 2021-12-27 21:26   ` Philippe Mathieu-Daudé
  2021-12-28 14:25   ` Denis Lisov
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-27 21:26 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: serat, dlenski, mst

On 12/27/21 20:31, Igor Mammedov wrote:
> if QEMU is started with used provided SLIC table blob,
> 
>   -acpitable sig=SLIC,oem_id='CRASH ',oem_table_id="ME",oem_rev=00002210,asl_compiler_id="",asl_compiler_rev=00000000,data=/dev/null
> it will assert with:
> 
>   hw/acpi/aml-build.c:61:build_append_padded_str: assertion failed: (len <= maxlen)
> 
> and following backtrace:
> 
>   ...
>   build_append_padded_str (array=0x555556afe320, str=0x555556afdb2e "CRASH ME", maxlen=0x6, pad=0x20) at hw/acpi/aml-build.c:61
>   acpi_table_begin (desc=0x7fffffffd1b0, array=0x555556afe320) at hw/acpi/aml-build.c:1727
>   build_fadt (tbl=0x555556afe320, linker=0x555557ca3830, f=0x7fffffffd318, oem_id=0x555556afdb2e "CRASH ME", oem_table_id=0x555556afdb34 "ME") at hw/acpi/aml-build.c:2064
>   ...
> 
> which happens due to acpi_table_begin() expecting NULL terminated
> oem_id and oem_table_id strings, which is normally the case, but
> in case of user provided SLIC table, oem_id points to table's blob
> directly and as result oem_id became longer than expected.
> 
> Fix issue by handling oem_id consistently and make acpi_get_slic_oem()
> return NULL terminated strings.
> 
> PS:
> After [1] refactoring, oem_id semantics became inconsistent, where
> NULL terminated string was coming from machine and old way pointer
> into byte array coming from -acpitable option. That used to work
> since build_header() wasn't expecting NULL terminated string and
> blindly copied the 1st 6 bytes only.
> 
> However commit [2] broke that by replacing build_header() with
> acpi_table_begin(), which was expecting NULL terminated string
> and was checking oem_id size.
> 
> 1) 602b45820 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> 2)
> Fixes: 4b56e1e4eb08 ("acpi: build_fadt: use acpi_table_begin()/acpi_table_end() instead of build_header()")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/786
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/core.c       | 4 ++--
>  hw/i386/acpi-build.c | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
  2021-12-27 21:26   ` Philippe Mathieu-Daudé
@ 2021-12-28 14:25   ` Denis Lisov
  2021-12-30 22:30   ` Alexander Tsoy
  2022-01-03  7:15   ` Igor Mammedov
  3 siblings, 0 replies; 9+ messages in thread
From: Denis Lisov @ 2021-12-28 14:25 UTC (permalink / raw)
  To: qemu-devel; +Cc: Igor Mammedov, dlenski, mst

On Monday, 27 December 2021 22:31:17 MSK Igor Mammedov wrote:
> if QEMU is started with used provided SLIC table blob,
> 
>   -acpitable sig=SLIC,oem_id='CRASH
> ',oem_table_id="ME",oem_rev=00002210,asl_compiler_id="",asl_compiler_rev=00
> 000000,data=/dev/null it will assert with:
> 
>   hw/acpi/aml-build.c:61:build_append_padded_str: assertion failed: (len <=
> maxlen)
> 
> and following backtrace:
> 
>   ...
>   build_append_padded_str (array=0x555556afe320, str=0x555556afdb2e "CRASH
> ME", maxlen=0x6, pad=0x20) at hw/acpi/aml-build.c:61 acpi_table_begin
> (desc=0x7fffffffd1b0, array=0x555556afe320) at hw/acpi/aml-build.c:1727
> build_fadt (tbl=0x555556afe320, linker=0x555557ca3830, f=0x7fffffffd318,
> oem_id=0x555556afdb2e "CRASH ME", oem_table_id=0x555556afdb34 "ME") at
> hw/acpi/aml-build.c:2064 ...
> 
> which happens due to acpi_table_begin() expecting NULL terminated
> oem_id and oem_table_id strings, which is normally the case, but
> in case of user provided SLIC table, oem_id points to table's blob
> directly and as result oem_id became longer than expected.
> 
> Fix issue by handling oem_id consistently and make acpi_get_slic_oem()
> return NULL terminated strings.
> 
> PS:
> After [1] refactoring, oem_id semantics became inconsistent, where
> NULL terminated string was coming from machine and old way pointer
> into byte array coming from -acpitable option. That used to work
> since build_header() wasn't expecting NULL terminated string and
> blindly copied the 1st 6 bytes only.
> 
> However commit [2] broke that by replacing build_header() with
> acpi_table_begin(), which was expecting NULL terminated string
> and was checking oem_id size.
> 
> 1) 602b45820 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> 2)
> Fixes: 4b56e1e4eb08 ("acpi: build_fadt: use
> acpi_table_begin()/acpi_table_end() instead of build_header()") Resolves:
> https://gitlab.com/qemu-project/qemu/-/issues/786
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/core.c       | 4 ++--
>  hw/i386/acpi-build.c | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 1e004d0078..3e811bf03c 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -345,8 +345,8 @@ int acpi_get_slic_oem(AcpiSlicOem *oem)
>          struct acpi_table_header *hdr = (void *)(u - sizeof(hdr->_length));
> 
>          if (memcmp(hdr->sig, "SLIC", 4) == 0) {
> -            oem->id = hdr->oem_id;
> -            oem->table_id = hdr->oem_table_id;
> +            oem->id = g_strndup(hdr->oem_id, 6);
> +            oem->table_id = g_strndup(hdr->oem_table_id, 8);
>              return 0;
>          }
>      }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 8383b83ee3..0234fe7588 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2723,6 +2723,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState
> *machine)
> 
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
> +    g_free(slic_oem.id);
> +    g_free(slic_oem.table_id);
>  }
> 
>  static void acpi_ram_update(MemoryRegion *mr, GArray *data)

Tested-by: Denis Lisov <dennis.lissov@gmail.com>




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
  2021-12-27 21:26   ` Philippe Mathieu-Daudé
  2021-12-28 14:25   ` Denis Lisov
@ 2021-12-30 22:30   ` Alexander Tsoy
  2022-01-03  7:15   ` Igor Mammedov
  3 siblings, 0 replies; 9+ messages in thread
From: Alexander Tsoy @ 2021-12-30 22:30 UTC (permalink / raw)
  To: Igor Mammedov, qemu-devel; +Cc: dlenski, mst

В Пн, 27/12/2021 в 14:31 -0500, Igor Mammedov пишет:
> if QEMU is started with used provided SLIC table blob,
> 
>   -acpitable sig=SLIC,oem_id='CRASH
> ',oem_table_id="ME",oem_rev=00002210,asl_compiler_id="",asl_compiler_re
> v=00000000,data=/dev/null
> it will assert with:
> 
>   hw/acpi/aml-build.c:61:build_append_padded_str: assertion failed:
> (len <= maxlen)
> 
> and following backtrace:
> 
>   ...
>   build_append_padded_str (array=0x555556afe320, str=0x555556afdb2e
> "CRASH ME", maxlen=0x6, pad=0x20) at hw/acpi/aml-build.c:61
>   acpi_table_begin (desc=0x7fffffffd1b0, array=0x555556afe320) at
> hw/acpi/aml-build.c:1727
>   build_fadt (tbl=0x555556afe320, linker=0x555557ca3830,
> f=0x7fffffffd318, oem_id=0x555556afdb2e "CRASH ME",
> oem_table_id=0x555556afdb34 "ME") at hw/acpi/aml-build.c:2064
>   ...
> 
> which happens due to acpi_table_begin() expecting NULL terminated
> oem_id and oem_table_id strings, which is normally the case, but
> in case of user provided SLIC table, oem_id points to table's blob
> directly and as result oem_id became longer than expected.
> 
> Fix issue by handling oem_id consistently and make acpi_get_slic_oem()
> return NULL terminated strings.
> 
> PS:
> After [1] refactoring, oem_id semantics became inconsistent, where
> NULL terminated string was coming from machine and old way pointer
> into byte array coming from -acpitable option. That used to work
> since build_header() wasn't expecting NULL terminated string and
> blindly copied the 1st 6 bytes only.
> 
> However commit [2] broke that by replacing build_header() with
> acpi_table_begin(), which was expecting NULL terminated string
> and was checking oem_id size.
> 
> 1) 602b45820 ("acpi: Permit OEM ID and OEM table ID fields to be
> changed")
> 2)
> Fixes: 4b56e1e4eb08 ("acpi: build_fadt: use
> acpi_table_begin()/acpi_table_end() instead of build_header()")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/786
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/core.c       | 4 ++--
>  hw/i386/acpi-build.c | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 1e004d0078..3e811bf03c 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -345,8 +345,8 @@ int acpi_get_slic_oem(AcpiSlicOem *oem)
>          struct acpi_table_header *hdr = (void *)(u - sizeof(hdr-
> >_length));
>  
>          if (memcmp(hdr->sig, "SLIC", 4) == 0) {
> -            oem->id = hdr->oem_id;
> -            oem->table_id = hdr->oem_table_id;
> +            oem->id = g_strndup(hdr->oem_id, 6);
> +            oem->table_id = g_strndup(hdr->oem_table_id, 8);
>              return 0;
>          }
>      }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 8383b83ee3..0234fe7588 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2723,6 +2723,8 @@ void acpi_build(AcpiBuildTables *tables,
> MachineState *machine)
>  
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
> +    g_free(slic_oem.id);
> +    g_free(slic_oem.table_id);
>  }
>  
>  static void acpi_ram_update(MemoryRegion *mr, GArray *data)

Tested-by: Alexander Tsoy <alexander@tsoy.me>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table
  2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
                     ` (2 preceding siblings ...)
  2021-12-30 22:30   ` Alexander Tsoy
@ 2022-01-03  7:15   ` Igor Mammedov
  3 siblings, 0 replies; 9+ messages in thread
From: Igor Mammedov @ 2022-01-03  7:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-stable, mst

CCing, qemu-stable@

On Mon, 27 Dec 2021 14:31:17 -0500
Igor Mammedov <imammedo@redhat.com> wrote:

> if QEMU is started with used provided SLIC table blob,
> 
>   -acpitable sig=SLIC,oem_id='CRASH ',oem_table_id="ME",oem_rev=00002210,asl_compiler_id="",asl_compiler_rev=00000000,data=/dev/null
> it will assert with:
> 
>   hw/acpi/aml-build.c:61:build_append_padded_str: assertion failed: (len <= maxlen)
> 
> and following backtrace:
> 
>   ...
>   build_append_padded_str (array=0x555556afe320, str=0x555556afdb2e "CRASH ME", maxlen=0x6, pad=0x20) at hw/acpi/aml-build.c:61
>   acpi_table_begin (desc=0x7fffffffd1b0, array=0x555556afe320) at hw/acpi/aml-build.c:1727
>   build_fadt (tbl=0x555556afe320, linker=0x555557ca3830, f=0x7fffffffd318, oem_id=0x555556afdb2e "CRASH ME", oem_table_id=0x555556afdb34 "ME") at hw/acpi/aml-build.c:2064
>   ...
> 
> which happens due to acpi_table_begin() expecting NULL terminated
> oem_id and oem_table_id strings, which is normally the case, but
> in case of user provided SLIC table, oem_id points to table's blob
> directly and as result oem_id became longer than expected.
> 
> Fix issue by handling oem_id consistently and make acpi_get_slic_oem()
> return NULL terminated strings.
> 
> PS:
> After [1] refactoring, oem_id semantics became inconsistent, where
> NULL terminated string was coming from machine and old way pointer
> into byte array coming from -acpitable option. That used to work
> since build_header() wasn't expecting NULL terminated string and
> blindly copied the 1st 6 bytes only.
> 
> However commit [2] broke that by replacing build_header() with
> acpi_table_begin(), which was expecting NULL terminated string
> and was checking oem_id size.
> 
> 1) 602b45820 ("acpi: Permit OEM ID and OEM table ID fields to be changed")
> 2)
> Fixes: 4b56e1e4eb08 ("acpi: build_fadt: use acpi_table_begin()/acpi_table_end() instead of build_header()")
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/786
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/core.c       | 4 ++--
>  hw/i386/acpi-build.c | 2 ++
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 1e004d0078..3e811bf03c 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -345,8 +345,8 @@ int acpi_get_slic_oem(AcpiSlicOem *oem)
>          struct acpi_table_header *hdr = (void *)(u - sizeof(hdr->_length));
>  
>          if (memcmp(hdr->sig, "SLIC", 4) == 0) {
> -            oem->id = hdr->oem_id;
> -            oem->table_id = hdr->oem_table_id;
> +            oem->id = g_strndup(hdr->oem_id, 6);
> +            oem->table_id = g_strndup(hdr->oem_table_id, 8);
>              return 0;
>          }
>      }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 8383b83ee3..0234fe7588 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -2723,6 +2723,8 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>  
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
> +    g_free(slic_oem.id);
> +    g_free(slic_oem.table_id);
>  }
>  
>  static void acpi_ram_update(MemoryRegion *mr, GArray *data)



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-01-03  7:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-27 19:31 [PATCH 0/4] Fix -acpitable regression Igor Mammedov
2021-12-27 19:31 ` [PATCH 1/4] acpi: fix QEMU crash when started with SLIC table Igor Mammedov
2021-12-27 21:26   ` Philippe Mathieu-Daudé
2021-12-28 14:25   ` Denis Lisov
2021-12-30 22:30   ` Alexander Tsoy
2022-01-03  7:15   ` Igor Mammedov
2021-12-27 19:31 ` [PATCH 2/4] tests: acpi: whitelist expected blobs before changing them Igor Mammedov
2021-12-27 19:31 ` [PATCH 3/4] tests: acpi: add SLIC table test Igor Mammedov
2021-12-27 19:31 ` [PATCH 4/4] tests: acpi: SLIC: update expected blobs Igor Mammedov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.