[PATCH v2 0/9] hw/sd/sdcard: Fix CVE-2020-13253

[PATCH v2 0/9] hw/sd/sdcard: Fix CVE-2020-13253

[Philippe Mathieu-Daudé]

This series fixes CVE-2020-13253 by only allowing SD card image
sizes power of 2, and not switching to SEND_DATA state when the
address is invalid (out of range).

Patches missing review:
 3: boot_linux: Tag tests using a SD card with 'device:sd'
 4: boot_linux: Expand SD card image to power of 2
 7: hw/sd/sdcard: Do not allow invalid SD card sizes

Since v1:
Fixes issue due to image not power of 2:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg720737.html

Supersedes: <20200707132116.26207-1-f4bug@amsat.org>

Niek Linnenbank (1):
  docs/orangepi: Add instructions for resizing SD image to power of two

Philippe Mathieu-Daudé (8):
  MAINTAINERS: Cc qemu-block mailing list
  tests/acceptance/boot_linux: Tag tests using a SD card with
    'device:sd'
  tests/acceptance/boot_linux: Expand SD card image to power of 2
  hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
  hw/sd/sdcard: Simplify realize() a bit
  hw/sd/sdcard: Do not allow invalid SD card sizes
  hw/sd/sdcard: Update coding style to make checkpatch.pl happy
  hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

 docs/system/arm/orangepi.rst           | 16 ++++-
 hw/sd/sd.c                             | 86 ++++++++++++++++++++------
 MAINTAINERS                            |  1 +
 tests/acceptance/boot_linux_console.py | 30 ++++++---
 4 files changed, 102 insertions(+), 31 deletions(-)

-- 
2.21.3

[PATCH v2 1/9] MAINTAINERS: Cc qemu-block mailing list

[Philippe Mathieu-Daudé]

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

We forgot to include the qemu-block mailing list while adding
this section in commit 076a0fc32a7. Fix this.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Message-Id: <20200630133912.9428-2-f4bug@amsat.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fe8139f367..d9c71d0bb3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1674,6 +1674,7 @@ F: hw/ssi/xilinx_*
 
 SD (Secure Card)
 M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+L: qemu-block@nongnu.org
 S: Odd Fixes
 F: include/hw/sd/sd*
 F: hw/sd/core.c
-- 
2.21.3

[PATCH v2 2/9] docs/orangepi: Add instructions for resizing SD image to power of two

[Philippe Mathieu-Daudé]

From: Niek Linnenbank <nieklinnenbank@gmail.com>

SD cards need to have a size of a power of two.
Update the Orange Pi machine documentation to include
instructions for resizing downloaded images using the
qemu-img command.

Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200712183708.15450-1-nieklinnenbank@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 docs/system/arm/orangepi.rst | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/docs/system/arm/orangepi.rst b/docs/system/arm/orangepi.rst
index c41adad488..6f23907fb6 100644
--- a/docs/system/arm/orangepi.rst
+++ b/docs/system/arm/orangepi.rst
@@ -127,6 +127,16 @@ can be downloaded from:
 Alternatively, you can also choose to build you own image with buildroot
 using the orangepi_pc_defconfig. Also see https://buildroot.org for more information.
 
+When using an image as an SD card, it must be resized to a power of two. This can be
+done with the qemu-img command. It is recommended to only increase the image size
+instead of shrinking it to a power of two, to avoid loss of data. For example,
+to prepare a downloaded Armbian image, first extract it and then increase
+its size to one gigabyte as follows:
+
+.. code-block:: bash
+
+  $ qemu-img resize Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img 1G
+
 You can choose to attach the selected image either as an SD card or as USB mass storage.
 For example, to boot using the Orange Pi PC Debian image on SD card, simply add the -sd
 argument and provide the proper root= kernel parameter:
@@ -213,12 +223,12 @@ Next, unzip the NetBSD image and write the U-Boot binary including SPL using:
   $ dd if=/path/to/u-boot-sunxi-with-spl.bin of=armv7.img bs=1024 seek=8 conv=notrunc
 
 Finally, before starting the machine the SD image must be extended such
-that the NetBSD kernel will not conclude the NetBSD partition is larger than
-the emulated SD card:
+that the size of the SD image is a power of two and that the NetBSD kernel
+will not conclude the NetBSD partition is larger than the emulated SD card:
 
 .. code-block:: bash
 
-  $ dd if=/dev/zero bs=1M count=64 >> armv7.img
+  $ qemu-img resize armv7.img 2G
 
 Start the machine using the following command:
 
-- 
2.21.3

[PATCH v2 3/9] tests/acceptance/boot_linux: Tag tests using a SD card with 'device:sd'

[Philippe Mathieu-Daudé]

Avocado tags are handy to automatically select tests matching
the tags. Since these tests use a SD card, tag them.

We can run all the tests using a SD card at once with:

  $ avocado --show=app run -t u-boot tests/acceptance/
  $ AVOCADO_ALLOW_LARGE_STORAGE=ok \
    avocado --show=app \
      run -t device:sd tests/acceptance/
  Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
  Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic
  Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
   (1/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd: PASS (19.56 s)
   (2/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic: PASS (49.97 s)
   (3/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9: PASS (20.06 s)
  RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
  JOB TIME   : 90.02 s

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 tests/acceptance/boot_linux_console.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 3d02519660..b7e8858c2d 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -620,6 +620,7 @@ def test_arm_orangepi_sd(self):
         """
         :avocado: tags=arch:arm
         :avocado: tags=machine:orangepi-pc
+        :avocado: tags=device:sd
         """
         deb_url = ('https://apt.armbian.com/pool/main/l/'
                    'linux-4.20.7-sunxi/linux-image-dev-sunxi_5.75_armhf.deb')
@@ -669,6 +670,7 @@ def test_arm_orangepi_bionic(self):
         """
         :avocado: tags=arch:arm
         :avocado: tags=machine:orangepi-pc
+        :avocado: tags=device:sd
         """
 
         # This test download a 196MB compressed image and expand it to 932MB...
@@ -710,6 +712,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
         """
         :avocado: tags=arch:arm
         :avocado: tags=machine:orangepi-pc
+        :avocado: tags=device:sd
         """
         # This test download a 304MB compressed image and expand it to 1.3GB...
         deb_url = ('http://snapshot.debian.org/archive/debian/'
-- 
2.21.3

[PATCH v2 4/9] tests/acceptance/boot_linux: Expand SD card image to power of 2

[Philippe Mathieu-Daudé]

In few commits we won't allow SD card images with invalid size
(not aligned to a power of 2). Prepare the tests: add the
pow2ceil() and image_pow2ceil_expand() methods and resize the
images (expanding) of the tests using SD cards.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1: Addressed review comments
- truncate -> expand reword (Alistair Francis)
- expand after uncompress (Niek Linnenbank)
---
 tests/acceptance/boot_linux_console.py | 27 +++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index b7e8858c2d..8f2a6aa8a4 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -28,6 +28,18 @@
 except CmdNotFoundError:
     P7ZIP_AVAILABLE = False
 
+# round up to next power of 2
+def pow2ceil(x):
+    return 1 if x == 0 else 2**(x - 1).bit_length()
+
+# expand file size to next power of 2
+def image_pow2ceil_expand(path):
+        size = os.path.getsize(path)
+        size_aligned = pow2ceil(size)
+        if size != size_aligned:
+            with open(path, 'ab+') as fd:
+                fd.truncate(size_aligned)
+
 class LinuxKernelTest(Test):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
 
@@ -636,6 +648,7 @@ def test_arm_orangepi_sd(self):
         rootfs_path_xz = self.fetch_asset(rootfs_url, asset_hash=rootfs_hash)
         rootfs_path = os.path.join(self.workdir, 'rootfs.cpio')
         archive.lzma_uncompress(rootfs_path_xz, rootfs_path)
+        image_pow2ceil_expand(rootfs_path)
 
         self.vm.set_console()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
@@ -673,7 +686,7 @@ def test_arm_orangepi_bionic(self):
         :avocado: tags=device:sd
         """
 
-        # This test download a 196MB compressed image and expand it to 932MB...
+        # This test download a 196MB compressed image and expand it to 1GB
         image_url = ('https://dl.armbian.com/orangepipc/archive/'
                      'Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.7z')
         image_hash = '196a8ffb72b0123d92cea4a070894813d305c71e'
@@ -681,6 +694,7 @@ def test_arm_orangepi_bionic(self):
         image_name = 'Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img'
         image_path = os.path.join(self.workdir, image_name)
         process.run("7z e -o%s %s" % (self.workdir, image_path_7z))
+        image_pow2ceil_expand(image_path)
 
         self.vm.set_console()
         self.vm.add_args('-drive', 'file=' + image_path + ',if=sd,format=raw',
@@ -714,7 +728,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
         :avocado: tags=machine:orangepi-pc
         :avocado: tags=device:sd
         """
-        # This test download a 304MB compressed image and expand it to 1.3GB...
+        # This test download a 304MB compressed image and expand it to 2GB
         deb_url = ('http://snapshot.debian.org/archive/debian/'
                    '20200108T145233Z/pool/main/u/u-boot/'
                    'u-boot-sunxi_2020.01%2Bdfsg-1_armhf.deb')
@@ -731,8 +745,9 @@ def test_arm_orangepi_uboot_netbsd9(self):
         image_hash = '2babb29d36d8360adcb39c09e31060945259917a'
         image_path_gz = self.fetch_asset(image_url, asset_hash=image_hash)
         image_path = os.path.join(self.workdir, 'armv7.img')
-        image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
         archive.gzip_uncompress(image_path_gz, image_path)
+        image_pow2ceil_expand(image_path)
+        image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
 
         # dd if=u-boot-sunxi-with-spl.bin of=armv7.img bs=1K seek=8 conv=notrunc
         with open(uboot_path, 'rb') as f_in:
@@ -740,12 +755,6 @@ def test_arm_orangepi_uboot_netbsd9(self):
                 f_out.seek(8 * 1024)
                 shutil.copyfileobj(f_in, f_out)
 
-                # Extend image, to avoid that NetBSD thinks the partition
-                # inside the image is larger than device size itself
-                f_out.seek(0, 2)
-                f_out.seek(64 * 1024 * 1024, 1)
-                f_out.write(bytearray([0x00]))
-
         self.vm.set_console()
         self.vm.add_args('-nic', 'user',
                          '-drive', image_drive_args,
-- 
2.21.3

[PATCH v2 5/9] hw/sd/sdcard: Restrict Class 6 commands to SCSD cards

[Philippe Mathieu-Daudé]

Only SCSD cards support Class 6 (Block Oriented Write Protection)
commands.

  "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"

  4.3.14 Command Functional Difference in Card Capacity Types

  * Write Protected Group

  SDHC and SDXC do not support write-protected groups. Issuing
  CMD28, CMD29 and CMD30 generates the ILLEGAL_COMMAND error.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200630133912.9428-7-f4bug@amsat.org>
---
 hw/sd/sd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5137168d66..1cc16bfd31 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -920,6 +920,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         sd->multi_blk_cnt = 0;
     }
 
+    if (sd_cmd_class[req.cmd] == 6 && FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
+        /* Only Standard Capacity cards support class 6 commands */
+        return sd_illegal;
+    }
+
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
     case 0:	/* CMD0:   GO_IDLE_STATE */
-- 
2.21.3

[PATCH v2 6/9] hw/sd/sdcard: Simplify realize() a bit

[Philippe Mathieu-Daudé]

We don't need to check if sd->blk is set twice.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200630133912.9428-18-f4bug@amsat.org>
---
 hw/sd/sd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1cc16bfd31..edd60a09c0 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2105,12 +2105,12 @@ static void sd_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (sd->blk && blk_is_read_only(sd->blk)) {
-        error_setg(errp, "Cannot use read-only drive as SD card");
-        return;
-    }
-
     if (sd->blk) {
+        if (blk_is_read_only(sd->blk)) {
+            error_setg(errp, "Cannot use read-only drive as SD card");
+            return;
+        }
+
         ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
                            BLK_PERM_ALL, errp);
         if (ret < 0) {
-- 
2.21.3

[PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes

[Philippe Mathieu-Daudé]

QEMU allows to create SD card with unrealistic sizes. This could
work, but some guests (at least Linux) consider sizes that are not
a power of 2 as a firmware bug and fix the card size to the next
power of 2.

While the possibility to use small SD card images has been seen as
a feature, it became a bug with CVE-2020-13253, where the guest is
able to do OOB read/write accesses past the image size end.

In a pair of commits we will fix CVE-2020-13253 as:

    Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
    occurred and no data transfer is performed.

    Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
    occurred and no data transfer is performed.

    WP_VIOLATION errors are not modified: the error bit is set, we
    stay in receive-data state, wait for a stop command. All further
    data transfer is ignored. See the check on sd->card_status at the
    beginning of sd_read_data() and sd_write_data().

While this is the correct behavior, in case QEMU create smaller SD
cards, guests still try to access past the image size end, and QEMU
considers this is an invalid address, thus "all further data transfer
is ignored". This is wrong and make the guest looping until
eventually timeouts.

Fix by not allowing invalid SD card sizes (suggesting the expected
size as a hint):

  $ qemu-system-arm -M orangepi-pc -drive file=rootfs.ext2,if=sd,format=raw
  qemu-system-arm: Invalid SD card size: 60 MiB
  SD card size has to be a power of 2, e.g. 64 MiB.
  You can resize disk images with 'qemu-img resize <imagefile> <new-size>'
  (note that this will lose data if you make the image smaller than it currently is).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v1:
  Addressed Alistair & Peter comments (error_append_hint message)
---
 hw/sd/sd.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index edd60a09c0..5ab945dade 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -32,6 +32,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/units.h"
+#include "qemu/cutils.h"
 #include "hw/irq.h"
 #include "hw/registerfields.h"
 #include "sysemu/block-backend.h"
@@ -2106,11 +2107,35 @@ static void sd_realize(DeviceState *dev, Error **errp)
     }
 
     if (sd->blk) {
+        int64_t blk_size;
+
         if (blk_is_read_only(sd->blk)) {
             error_setg(errp, "Cannot use read-only drive as SD card");
             return;
         }
 
+        blk_size = blk_getlength(sd->blk);
+        if (blk_size > 0 && !is_power_of_2(blk_size)) {
+            int64_t blk_size_aligned = pow2ceil(blk_size);
+            char *blk_size_str;
+
+            blk_size_str = size_to_str(blk_size);
+            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
+            g_free(blk_size_str);
+
+            blk_size_str = size_to_str(blk_size_aligned);
+            error_append_hint(errp,
+                              "SD card size has to be a power of 2, e.g. %s.\n"
+                              "You can resize disk images with "
+                              "'qemu-img resize <imagefile> <new-size>'\n"
+                              "(note that this will lose data if you make the "
+                              "image smaller than it currently is).\n",
+                              blk_size_str);
+            g_free(blk_size_str);
+
+            return;
+        }
+
         ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
                            BLK_PERM_ALL, errp);
         if (ret < 0) {
-- 
2.21.3

[PATCH v2 8/9] hw/sd/sdcard: Update coding style to make checkpatch.pl happy

[Philippe Mathieu-Daudé]

To make the next commit easier to review, clean this code first.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200630133912.9428-3-f4bug@amsat.org>
---
 hw/sd/sd.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 5ab945dade..0f048358ab 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1175,8 +1175,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_start = addr;
             sd->data_offset = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+            }
             return sd_r1;
 
         default:
@@ -1191,8 +1192,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_start = addr;
             sd->data_offset = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+            }
             return sd_r1;
 
         default:
@@ -1237,12 +1239,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
-            if (sd_wp_addr(sd, sd->data_start))
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
                 sd->card_status |= WP_VIOLATION;
-            if (sd->csd[14] & 0x30)
+            }
+            if (sd->csd[14] & 0x30) {
                 sd->card_status |= WP_VIOLATION;
+            }
             return sd_r1;
 
         default:
@@ -1261,12 +1266,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
-            if (sd_wp_addr(sd, sd->data_start))
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
                 sd->card_status |= WP_VIOLATION;
-            if (sd->csd[14] & 0x30)
+            }
+            if (sd->csd[14] & 0x30) {
                 sd->card_status |= WP_VIOLATION;
+            }
             return sd_r1;
 
         default:
-- 
2.21.3

[PATCH v2 9/9] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

Only move the state machine to ReceivingData if there is no
pending error. This avoids later OOB access while processing
commands queued.

  "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"

  4.3.3 Data Read

  Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
  occurred and no data transfer is performed.

  4.3.4 Data Write

  Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
  occurred and no data transfer is performed.

WP_VIOLATION errors are not modified: the error bit is set, we
stay in receive-data state, wait for a stop command. All further
data transfer is ignored. See the check on sd->card_status at the
beginning of sd_read_data() and sd_write_data().

Fixes: CVE-2020-13253
Cc: Prasad J Pandit <pjp@fedoraproject.org>
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Buglink: https://bugs.launchpad.net/qemu/+bug/1880822
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Message-Id: <20200630133912.9428-6-f4bug@amsat.org>
---
 hw/sd/sd.c | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0f048358ab..29de05f576 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1171,13 +1171,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 17:	/* CMD17:  READ_SINGLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
+
+            if (addr + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+
             sd->state = sd_sendingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
             return sd_r1;
 
         default:
@@ -1188,13 +1190,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
+
+            if (addr + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+
             sd->state = sd_sendingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
             return sd_r1;
 
         default:
@@ -1234,14 +1238,17 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             /* Writing in SPI mode not implemented.  */
             if (sd->spi)
                 break;
+
+            if (addr + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+
             sd->state = sd_receivingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
             if (sd_wp_addr(sd, sd->data_start)) {
                 sd->card_status |= WP_VIOLATION;
             }
@@ -1261,14 +1268,17 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             /* Writing in SPI mode not implemented.  */
             if (sd->spi)
                 break;
+
+            if (addr + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+
             sd->state = sd_receivingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
             if (sd_wp_addr(sd, sd->data_start)) {
                 sd->card_status |= WP_VIOLATION;
             }
-- 
2.21.3

Re: [PATCH v2 3/9] tests/acceptance/boot_linux: Tag tests using a SD card with 'device:sd'

[Alistair Francis]

On Mon, Jul 13, 2020 at 11:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Avocado tags are handy to automatically select tests matching
> the tags. Since these tests use a SD card, tag them.
>
> We can run all the tests using a SD card at once with:
>
>   $ avocado --show=app run -t u-boot tests/acceptance/
>   $ AVOCADO_ALLOW_LARGE_STORAGE=ok \
>     avocado --show=app \
>       run -t device:sd tests/acceptance/
>   Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
>   Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic
>   Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
>    (1/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd: PASS (19.56 s)
>    (2/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic: PASS (49.97 s)
>    (3/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9: PASS (20.06 s)
>   RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>   JOB TIME   : 90.02 s
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  tests/acceptance/boot_linux_console.py | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 3d02519660..b7e8858c2d 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -620,6 +620,7 @@ def test_arm_orangepi_sd(self):
>          """
>          :avocado: tags=arch:arm
>          :avocado: tags=machine:orangepi-pc
> +        :avocado: tags=device:sd
>          """
>          deb_url = ('https://apt.armbian.com/pool/main/l/'
>                     'linux-4.20.7-sunxi/linux-image-dev-sunxi_5.75_armhf.deb')
> @@ -669,6 +670,7 @@ def test_arm_orangepi_bionic(self):
>          """
>          :avocado: tags=arch:arm
>          :avocado: tags=machine:orangepi-pc
> +        :avocado: tags=device:sd
>          """
>
>          # This test download a 196MB compressed image and expand it to 932MB...
> @@ -710,6 +712,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
>          """
>          :avocado: tags=arch:arm
>          :avocado: tags=machine:orangepi-pc
> +        :avocado: tags=device:sd
>          """
>          # This test download a 304MB compressed image and expand it to 1.3GB...
>          deb_url = ('http://snapshot.debian.org/archive/debian/'
> --
> 2.21.3
>
>

Re: [PATCH v2 2/9] docs/orangepi: Add instructions for resizing SD image to power of two

[Alistair Francis]

On Mon, Jul 13, 2020 at 11:32 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Niek Linnenbank <nieklinnenbank@gmail.com>
>
> SD cards need to have a size of a power of two.
> Update the Orange Pi machine documentation to include
> instructions for resizing downloaded images using the
> qemu-img command.
>
> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Message-Id: <20200712183708.15450-1-nieklinnenbank@gmail.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  docs/system/arm/orangepi.rst | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/docs/system/arm/orangepi.rst b/docs/system/arm/orangepi.rst
> index c41adad488..6f23907fb6 100644
> --- a/docs/system/arm/orangepi.rst
> +++ b/docs/system/arm/orangepi.rst
> @@ -127,6 +127,16 @@ can be downloaded from:
>  Alternatively, you can also choose to build you own image with buildroot
>  using the orangepi_pc_defconfig. Also see https://buildroot.org for more information.
>
> +When using an image as an SD card, it must be resized to a power of two. This can be
> +done with the qemu-img command. It is recommended to only increase the image size
> +instead of shrinking it to a power of two, to avoid loss of data. For example,
> +to prepare a downloaded Armbian image, first extract it and then increase
> +its size to one gigabyte as follows:
> +
> +.. code-block:: bash
> +
> +  $ qemu-img resize Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img 1G
> +
>  You can choose to attach the selected image either as an SD card or as USB mass storage.
>  For example, to boot using the Orange Pi PC Debian image on SD card, simply add the -sd
>  argument and provide the proper root= kernel parameter:
> @@ -213,12 +223,12 @@ Next, unzip the NetBSD image and write the U-Boot binary including SPL using:
>    $ dd if=/path/to/u-boot-sunxi-with-spl.bin of=armv7.img bs=1024 seek=8 conv=notrunc
>
>  Finally, before starting the machine the SD image must be extended such
> -that the NetBSD kernel will not conclude the NetBSD partition is larger than
> -the emulated SD card:
> +that the size of the SD image is a power of two and that the NetBSD kernel
> +will not conclude the NetBSD partition is larger than the emulated SD card:
>
>  .. code-block:: bash
>
> -  $ dd if=/dev/zero bs=1M count=64 >> armv7.img
> +  $ qemu-img resize armv7.img 2G
>
>  Start the machine using the following command:
>
> --
> 2.21.3
>
>

Re: [PATCH v2 4/9] tests/acceptance/boot_linux: Expand SD card image to power of 2

[Alistair Francis]

On Mon, Jul 13, 2020 at 11:34 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> In few commits we won't allow SD card images with invalid size
> (not aligned to a power of 2). Prepare the tests: add the
> pow2ceil() and image_pow2ceil_expand() methods and resize the
> images (expanding) of the tests using SD cards.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Since v1: Addressed review comments
> - truncate -> expand reword (Alistair Francis)
> - expand after uncompress (Niek Linnenbank)
> ---
>  tests/acceptance/boot_linux_console.py | 27 +++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index b7e8858c2d..8f2a6aa8a4 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -28,6 +28,18 @@
>  except CmdNotFoundError:
>      P7ZIP_AVAILABLE = False
>
> +# round up to next power of 2
> +def pow2ceil(x):
> +    return 1 if x == 0 else 2**(x - 1).bit_length()
> +
> +# expand file size to next power of 2
> +def image_pow2ceil_expand(path):
> +        size = os.path.getsize(path)
> +        size_aligned = pow2ceil(size)
> +        if size != size_aligned:
> +            with open(path, 'ab+') as fd:
> +                fd.truncate(size_aligned)
> +
>  class LinuxKernelTest(Test):
>      KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>
> @@ -636,6 +648,7 @@ def test_arm_orangepi_sd(self):
>          rootfs_path_xz = self.fetch_asset(rootfs_url, asset_hash=rootfs_hash)
>          rootfs_path = os.path.join(self.workdir, 'rootfs.cpio')
>          archive.lzma_uncompress(rootfs_path_xz, rootfs_path)
> +        image_pow2ceil_expand(rootfs_path)
>
>          self.vm.set_console()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -673,7 +686,7 @@ def test_arm_orangepi_bionic(self):
>          :avocado: tags=device:sd
>          """
>
> -        # This test download a 196MB compressed image and expand it to 932MB...
> +        # This test download a 196MB compressed image and expand it to 1GB
>          image_url = ('https://dl.armbian.com/orangepipc/archive/'
>                       'Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.7z')
>          image_hash = '196a8ffb72b0123d92cea4a070894813d305c71e'
> @@ -681,6 +694,7 @@ def test_arm_orangepi_bionic(self):
>          image_name = 'Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.img'
>          image_path = os.path.join(self.workdir, image_name)
>          process.run("7z e -o%s %s" % (self.workdir, image_path_7z))
> +        image_pow2ceil_expand(image_path)
>
>          self.vm.set_console()
>          self.vm.add_args('-drive', 'file=' + image_path + ',if=sd,format=raw',
> @@ -714,7 +728,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
>          :avocado: tags=machine:orangepi-pc
>          :avocado: tags=device:sd
>          """
> -        # This test download a 304MB compressed image and expand it to 1.3GB...
> +        # This test download a 304MB compressed image and expand it to 2GB
>          deb_url = ('http://snapshot.debian.org/archive/debian/'
>                     '20200108T145233Z/pool/main/u/u-boot/'
>                     'u-boot-sunxi_2020.01%2Bdfsg-1_armhf.deb')
> @@ -731,8 +745,9 @@ def test_arm_orangepi_uboot_netbsd9(self):
>          image_hash = '2babb29d36d8360adcb39c09e31060945259917a'
>          image_path_gz = self.fetch_asset(image_url, asset_hash=image_hash)
>          image_path = os.path.join(self.workdir, 'armv7.img')
> -        image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
>          archive.gzip_uncompress(image_path_gz, image_path)
> +        image_pow2ceil_expand(image_path)
> +        image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
>
>          # dd if=u-boot-sunxi-with-spl.bin of=armv7.img bs=1K seek=8 conv=notrunc
>          with open(uboot_path, 'rb') as f_in:
> @@ -740,12 +755,6 @@ def test_arm_orangepi_uboot_netbsd9(self):
>                  f_out.seek(8 * 1024)
>                  shutil.copyfileobj(f_in, f_out)
>
> -                # Extend image, to avoid that NetBSD thinks the partition
> -                # inside the image is larger than device size itself
> -                f_out.seek(0, 2)
> -                f_out.seek(64 * 1024 * 1024, 1)
> -                f_out.write(bytearray([0x00]))
> -
>          self.vm.set_console()
>          self.vm.add_args('-nic', 'user',
>                           '-drive', image_drive_args,
> --
> 2.21.3
>
>

Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes

[Alistair Francis]

On Mon, Jul 13, 2020 at 11:33 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> QEMU allows to create SD card with unrealistic sizes. This could
> work, but some guests (at least Linux) consider sizes that are not
> a power of 2 as a firmware bug and fix the card size to the next
> power of 2.
>
> While the possibility to use small SD card images has been seen as
> a feature, it became a bug with CVE-2020-13253, where the guest is
> able to do OOB read/write accesses past the image size end.
>
> In a pair of commits we will fix CVE-2020-13253 as:
>
>     Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>     occurred and no data transfer is performed.
>
>     Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>     occurred and no data transfer is performed.
>
>     WP_VIOLATION errors are not modified: the error bit is set, we
>     stay in receive-data state, wait for a stop command. All further
>     data transfer is ignored. See the check on sd->card_status at the
>     beginning of sd_read_data() and sd_write_data().
>
> While this is the correct behavior, in case QEMU create smaller SD
> cards, guests still try to access past the image size end, and QEMU
> considers this is an invalid address, thus "all further data transfer
> is ignored". This is wrong and make the guest looping until
> eventually timeouts.
>
> Fix by not allowing invalid SD card sizes (suggesting the expected
> size as a hint):
>
>   $ qemu-system-arm -M orangepi-pc -drive file=rootfs.ext2,if=sd,format=raw
>   qemu-system-arm: Invalid SD card size: 60 MiB
>   SD card size has to be a power of 2, e.g. 64 MiB.
>   You can resize disk images with 'qemu-img resize <imagefile> <new-size>'
>   (note that this will lose data if you make the image smaller than it currently is).
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> Since v1:
>   Addressed Alistair & Peter comments (error_append_hint message)
> ---
>  hw/sd/sd.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index edd60a09c0..5ab945dade 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -32,6 +32,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qemu/units.h"
> +#include "qemu/cutils.h"
>  #include "hw/irq.h"
>  #include "hw/registerfields.h"
>  #include "sysemu/block-backend.h"
> @@ -2106,11 +2107,35 @@ static void sd_realize(DeviceState *dev, Error **errp)
>      }
>
>      if (sd->blk) {
> +        int64_t blk_size;
> +
>          if (blk_is_read_only(sd->blk)) {
>              error_setg(errp, "Cannot use read-only drive as SD card");
>              return;
>          }
>
> +        blk_size = blk_getlength(sd->blk);
> +        if (blk_size > 0 && !is_power_of_2(blk_size)) {
> +            int64_t blk_size_aligned = pow2ceil(blk_size);
> +            char *blk_size_str;
> +
> +            blk_size_str = size_to_str(blk_size);
> +            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
> +            g_free(blk_size_str);
> +
> +            blk_size_str = size_to_str(blk_size_aligned);
> +            error_append_hint(errp,
> +                              "SD card size has to be a power of 2, e.g. %s.\n"
> +                              "You can resize disk images with "
> +                              "'qemu-img resize <imagefile> <new-size>'\n"
> +                              "(note that this will lose data if you make the "
> +                              "image smaller than it currently is).\n",
> +                              blk_size_str);
> +            g_free(blk_size_str);
> +
> +            return;
> +        }
> +
>          ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>                             BLK_PERM_ALL, errp);
>          if (ret < 0) {
> --
> 2.21.3
>
>

Re: [PATCH v2 8/9] hw/sd/sdcard: Update coding style to make checkpatch.pl happy

[Alexander Bulekov]

On 200713 2032, Philippe Mathieu-Daudé wrote:
> To make the next commit easier to review, clean this code first.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Message-Id: <20200630133912.9428-3-f4bug@amsat.org>
> ---

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

>  hw/sd/sd.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 5ab945dade..0f048358ab 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1175,8 +1175,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>              sd->data_start = addr;
>              sd->data_offset = 0;
>  
> -            if (sd->data_start + sd->blk_len > sd->size)
> +            if (sd->data_start + sd->blk_len > sd->size) {
>                  sd->card_status |= ADDRESS_ERROR;
> +            }
>              return sd_r1;
>  
>          default:
> @@ -1191,8 +1192,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>              sd->data_start = addr;
>              sd->data_offset = 0;
>  
> -            if (sd->data_start + sd->blk_len > sd->size)
> +            if (sd->data_start + sd->blk_len > sd->size) {
>                  sd->card_status |= ADDRESS_ERROR;
> +            }
>              return sd_r1;
>  
>          default:
> @@ -1237,12 +1239,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>              sd->data_offset = 0;
>              sd->blk_written = 0;
>  
> -            if (sd->data_start + sd->blk_len > sd->size)
> +            if (sd->data_start + sd->blk_len > sd->size) {
>                  sd->card_status |= ADDRESS_ERROR;
> -            if (sd_wp_addr(sd, sd->data_start))
> +            }
> +            if (sd_wp_addr(sd, sd->data_start)) {
>                  sd->card_status |= WP_VIOLATION;
> -            if (sd->csd[14] & 0x30)
> +            }
> +            if (sd->csd[14] & 0x30) {
>                  sd->card_status |= WP_VIOLATION;
> +            }
>              return sd_r1;
>  
>          default:
> @@ -1261,12 +1266,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>              sd->data_offset = 0;
>              sd->blk_written = 0;
>  
> -            if (sd->data_start + sd->blk_len > sd->size)
> +            if (sd->data_start + sd->blk_len > sd->size) {
>                  sd->card_status |= ADDRESS_ERROR;
> -            if (sd_wp_addr(sd, sd->data_start))
> +            }
> +            if (sd_wp_addr(sd, sd->data_start)) {
>                  sd->card_status |= WP_VIOLATION;
> -            if (sd->csd[14] & 0x30)
> +            }
> +            if (sd->csd[14] & 0x30) {
>                  sd->card_status |= WP_VIOLATION;
> +            }
>              return sd_r1;
>  
>          default:
> -- 
> 2.21.3
> 

Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes

[Peter Maydell]

On Mon, 13 Jul 2020 at 19:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> QEMU allows to create SD card with unrealistic sizes. This could
> work, but some guests (at least Linux) consider sizes that are not
> a power of 2 as a firmware bug and fix the card size to the next
> power of 2.
>

> +            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
> +            g_free(blk_size_str);
> +
> +            blk_size_str = size_to_str(blk_size_aligned);
> +            error_append_hint(errp,
> +                              "SD card size has to be a power of 2, e.g. %s.\n"
> +                              "You can resize disk images with "
> +                              "'qemu-img resize <imagefile> <new-size>'\n"
> +                              "(note that this will lose data if you make the "
> +                              "image smaller than it currently is).\n",
> +                              blk_size_str);
> +            g_free(blk_size_str);

Some places that create multi-line hints with error_append_hint()
do it by calling it once per line, eg in target/arm/cpu64.c:
                error_setg(errp, "cannot disable sve128");
                error_append_hint(errp, "Disabling sve128 results in all "
                                  "vector lengths being disabled.\n");
                error_append_hint(errp, "With SVE enabled, at least one "
                                  "vector length must be enabled.\n");

Some places don't, eg in block/vhdx-log.c:
            error_append_hint(errp,  "To replay the log, run:\n"
                              "qemu-img check -r all '%s'\n",
                              bs->filename);

Most places terminate with a '\n', but some places don't, eg
crypto/block-luks.c:
       error_append_hint(errp, "Failed to write to keyslot %i", keyslot);
       return -1;

The documentation says
 * May be called multiple times.  The resulting hint should end with a
 * newline.

which isn't very clear -- you can call it multiple times, but
must you, if it's multiline?

I assume that "should end with a newline" means "must end
with a newline", and places like block-luks.c are bugs.

Markus, do you know what the intended API here is?

It looks like the implementation just tacks the hint
string onto the end of any existing hint string, in
which case multiple-line strings are fine and the same
behaviour as calling the function multiple times.
(I had assumed we might be accumulating an array of strings,
or requiring multiline strings to be multiple calls so we
could have the argument not need to be \n-terminated,
to match error_setg(), but both those assumptions
are obviously wrong.)

Anyway, I guess this multiline-message usage is something
we do already and it will DTRT, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM

Re: [PATCH v2 3/9] tests/acceptance/boot_linux: Tag tests using a SD card with 'device:sd'

[Cleber Rosa]

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

On Mon, Jul 13, 2020 at 08:32:03PM +0200, Philippe Mathieu-Daudé wrote:
> Avocado tags are handy to automatically select tests matching
> the tags. Since these tests use a SD card, tag them.
> 
> We can run all the tests using a SD card at once with:
> 
>   $ avocado --show=app run -t u-boot tests/acceptance/
>   $ AVOCADO_ALLOW_LARGE_STORAGE=ok \
>     avocado --show=app \
>       run -t device:sd tests/acceptance/
>   Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd
>   Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic
>   Fetching asset from tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9
>    (1/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_sd: PASS (19.56 s)
>    (2/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_bionic: PASS (49.97 s)
>    (3/3) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9: PASS (20.06 s)
>   RESULTS    : PASS 3 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0
>   JOB TIME   : 90.02 s
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Cleber Rosa <crosa@redhat.com>
Tested-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/9] tests/acceptance/boot_linux: Expand SD card image to power of 2

[Cleber Rosa]

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

On Mon, Jul 13, 2020 at 08:32:04PM +0200, Philippe Mathieu-Daudé wrote:
> In few commits we won't allow SD card images with invalid size
> (not aligned to a power of 2). Prepare the tests: add the
> pow2ceil() and image_pow2ceil_expand() methods and resize the
> images (expanding) of the tests using SD cards.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Since v1: Addressed review comments
> - truncate -> expand reword (Alistair Francis)
> - expand after uncompress (Niek Linnenbank)
> ---
>  tests/acceptance/boot_linux_console.py | 27 +++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index b7e8858c2d..8f2a6aa8a4 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -28,6 +28,18 @@
>  except CmdNotFoundError:
>      P7ZIP_AVAILABLE = False
>  
> +# round up to next power of 2
> +def pow2ceil(x):
> +    return 1 if x == 0 else 2**(x - 1).bit_length()
> +

Nitpick: turn the comment into a docstring.

Then, I was going to have a second nitpick about the method name, but
realized it was following qemu-common.h's implementation.

> +# expand file size to next power of 2
> +def image_pow2ceil_expand(path):
> +        size = os.path.getsize(path)
> +        size_aligned = pow2ceil(size)
> +        if size != size_aligned:
> +            with open(path, 'ab+') as fd:
> +                fd.truncate(size_aligned)
> +

Same nitpick comment about comment -> docstring here.

Either way,

Reviewed-by: Cleber Rosa <crosa@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Mon, 13 Jul 2020 at 19:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> QEMU allows to create SD card with unrealistic sizes. This could
>> work, but some guests (at least Linux) consider sizes that are not
>> a power of 2 as a firmware bug and fix the card size to the next
>> power of 2.
>>
>
>> +            error_setg(errp, "Invalid SD card size: %s", blk_size_str);
>> +            g_free(blk_size_str);
>> +
>> +            blk_size_str = size_to_str(blk_size_aligned);
>> +            error_append_hint(errp,
>> +                              "SD card size has to be a power of 2, e.g. %s.\n"
>> +                              "You can resize disk images with "
>> +                              "'qemu-img resize <imagefile> <new-size>'\n"
>> +                              "(note that this will lose data if you make the "
>> +                              "image smaller than it currently is).\n",
>> +                              blk_size_str);
>> +            g_free(blk_size_str);
>
> Some places that create multi-line hints with error_append_hint()
> do it by calling it once per line, eg in target/arm/cpu64.c:
>                 error_setg(errp, "cannot disable sve128");
>                 error_append_hint(errp, "Disabling sve128 results in all "
>                                   "vector lengths being disabled.\n");
>                 error_append_hint(errp, "With SVE enabled, at least one "
>                                   "vector length must be enabled.\n");
>
> Some places don't, eg in block/vhdx-log.c:
>             error_append_hint(errp,  "To replay the log, run:\n"
>                               "qemu-img check -r all '%s'\n",
>                               bs->filename);

Both fine.

> Most places terminate with a '\n', but some places don't, eg
> crypto/block-luks.c:
>        error_append_hint(errp, "Failed to write to keyslot %i", keyslot);
>        return -1;

This is a bug.

> The documentation says
>  * May be called multiple times.  The resulting hint should end with a
>  * newline.
>
> which isn't very clear -- you can call it multiple times, but
> must you, if it's multiline?

If that was required, my doc comment would demand it.

> I assume that "should end with a newline" means "must end
> with a newline", and places like block-luks.c are bugs.

If you construct a hint with multiple calls, individual calls need not
terminate with a newline, but the resulting hint still should.

I readily admit that my doc comments sometimes require a "lawyerly"
reading :)

> Markus, do you know what the intended API here is?
>
> It looks like the implementation just tacks the hint
> string onto the end of any existing hint string, in
> which case multiple-line strings are fine and the same
> behaviour as calling the function multiple times.

That's the intended behavior.

> (I had assumed we might be accumulating an array of strings,
> or requiring multiline strings to be multiple calls so we
> could have the argument not need to be \n-terminated,
> to match error_setg(), but both those assumptions
> are obviously wrong.)

Would also be workable, if slightly less flexible.

Yes, error_setg() and error_append_hint() differ on newlines.

error_setg() constructs an error message.  These get reported in a rigid
format that does not afford line breaks in the middle.

error_append_hint() builds up a hint.  Hints are free-form text.
Embedded line breaks are fine.

In both cases, I decided not to treat newline at the end any different
from newlines anywhere else.  Thus, no newlines at all with
error_setg(), and newlines after every line including the last one with
error_append_hint().

> Anyway, I guess this multiline-message usage is something
> we do already and it will DTRT, so

Correct.

Hope this helps!

> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
>
> thanks
> -- PMM

Re: [PATCH v2 4/9] tests/acceptance/boot_linux: Expand SD card image to power of 2

[Philippe Mathieu-Daudé]

On 7/14/20 5:22 AM, Cleber Rosa wrote:
> On Mon, Jul 13, 2020 at 08:32:04PM +0200, Philippe Mathieu-Daudé wrote:
>> In few commits we won't allow SD card images with invalid size
>> (not aligned to a power of 2). Prepare the tests: add the
>> pow2ceil() and image_pow2ceil_expand() methods and resize the
>> images (expanding) of the tests using SD cards.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Since v1: Addressed review comments
>> - truncate -> expand reword (Alistair Francis)
>> - expand after uncompress (Niek Linnenbank)
>> ---
>>  tests/acceptance/boot_linux_console.py | 27 +++++++++++++++++---------
>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
>> index b7e8858c2d..8f2a6aa8a4 100644
>> --- a/tests/acceptance/boot_linux_console.py
>> +++ b/tests/acceptance/boot_linux_console.py
>> @@ -28,6 +28,18 @@
>>  except CmdNotFoundError:
>>      P7ZIP_AVAILABLE = False
>>  
>> +# round up to next power of 2
>> +def pow2ceil(x):
>> +    return 1 if x == 0 else 2**(x - 1).bit_length()
>> +
> 
> Nitpick: turn the comment into a docstring.

OK will do.

> Then, I was going to have a second nitpick about the method name, but
> realized it was following qemu-common.h's implementation.
> 
>> +# expand file size to next power of 2
>> +def image_pow2ceil_expand(path):
>> +        size = os.path.getsize(path)
>> +        size_aligned = pow2ceil(size)
>> +        if size != size_aligned:
>> +            with open(path, 'ab+') as fd:
>> +                fd.truncate(size_aligned)
>> +
> 
> Same nitpick comment about comment -> docstring here.
> 
> Either way,
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>

Thanks!

Phil.

Re: [PATCH v2 0/9] hw/sd/sdcard: Fix CVE-2020-13253

[Philippe Mathieu-Daudé]

On 7/13/20 8:32 PM, Philippe Mathieu-Daudé wrote:
> This series fixes CVE-2020-13253 by only allowing SD card image
> sizes power of 2, and not switching to SEND_DATA state when the
> address is invalid (out of range).
> 
> Patches missing review:
>  3: boot_linux: Tag tests using a SD card with 'device:sd'
>  4: boot_linux: Expand SD card image to power of 2
>  7: hw/sd/sdcard: Do not allow invalid SD card sizes
> 
> Since v1:
> Fixes issue due to image not power of 2:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg720737.html
> 
> Supersedes: <20200707132116.26207-1-f4bug@amsat.org>
> 
> Niek Linnenbank (1):
>   docs/orangepi: Add instructions for resizing SD image to power of two
> 
> Philippe Mathieu-Daudé (8):
>   MAINTAINERS: Cc qemu-block mailing list
>   tests/acceptance/boot_linux: Tag tests using a SD card with
>     'device:sd'
>   tests/acceptance/boot_linux: Expand SD card image to power of 2
>   hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
>   hw/sd/sdcard: Simplify realize() a bit
>   hw/sd/sdcard: Do not allow invalid SD card sizes
>   hw/sd/sdcard: Update coding style to make checkpatch.pl happy
>   hw/sd/sdcard: Do not switch to ReceivingData if address is invalid
> 
>  docs/system/arm/orangepi.rst           | 16 ++++-
>  hw/sd/sd.c                             | 86 ++++++++++++++++++++------
>  MAINTAINERS                            |  1 +
>  tests/acceptance/boot_linux_console.py | 30 ++++++---
>  4 files changed, 102 insertions(+), 31 deletions(-)

Thanks for the reviews.

I addressed Cleber minor comment and will send a pull request shortly.

Phil.