[PATCH 0/2] hw/sd/sdcard: Fix CVE-2020-13253 (Do not allow invalid SD card sizes)

[PATCH 0/2] hw/sd/sdcard: Fix CVE-2020-13253 (Do not allow invalid SD card sizes)

[Philippe Mathieu-Daudé]

Part 1 is already reviewed:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html

However the CVE fix break Linux guests:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg720737.html

This series fixes that, by checking the SD card image size is
correct.

Based-on: <20200630133912.9428-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (2):
  tests/acceptance/boot_linux: Truncate SD card image to power of 2
  hw/sd/sdcard: Do not allow invalid SD card sizes

 hw/sd/sd.c                             | 16 ++++++++++++++++
 tests/acceptance/boot_linux_console.py | 15 +++++++++++++++
 2 files changed, 31 insertions(+)

-- 
2.21.3

[PATCH 1/2] tests/acceptance/boot_linux: Truncate SD card image to power of 2

[Philippe Mathieu-Daudé]

In the next commit 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_truncate() methods and truncate
the images of the tests using SD cards.

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

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 3d02519660..f4d4e3635f 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()
+
+# truncate file size to next power of 2
+def image_pow2ceil_truncate(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 '
 
@@ -635,6 +647,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_truncate(rootfs_path)
 
         self.vm.set_console()
         kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
@@ -679,6 +692,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_truncate(image_path)
 
         self.vm.set_console()
         self.vm.add_args('-drive', 'file=' + image_path + ',if=sd,format=raw',
@@ -728,6 +742,7 @@ 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_pow2ceil_truncate(image_path)
         image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
         archive.gzip_uncompress(image_path_gz, image_path)
 
-- 
2.21.3

Re: [PATCH 1/2] tests/acceptance/boot_linux: Truncate SD card image to power of 2

[Alistair Francis]

n Tue, Jul 7, 2020 at 6:21 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> In the next commit 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_truncate() methods and truncate
> the images of the tests using SD cards.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 3d02519660..f4d4e3635f 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()
> +
> +# truncate file size to next power of 2
> +def image_pow2ceil_truncate(path):
> +        size = os.path.getsize(path)
> +        size_aligned = pow2ceil(size)
> +        if size != size_aligned:
> +            with open(path, 'ab+') as fd:
> +                fd.truncate(size_aligned)

Why truncate the image, can't we expand it instead?

Alistair

> +
>  class LinuxKernelTest(Test):
>      KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>
> @@ -635,6 +647,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_truncate(rootfs_path)
>
>          self.vm.set_console()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -679,6 +692,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_truncate(image_path)
>
>          self.vm.set_console()
>          self.vm.add_args('-drive', 'file=' + image_path + ',if=sd,format=raw',
> @@ -728,6 +742,7 @@ 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_pow2ceil_truncate(image_path)
>          image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
>          archive.gzip_uncompress(image_path_gz, image_path)
>
> --
> 2.21.3
>
>

Re: [PATCH 1/2] tests/acceptance/boot_linux: Truncate SD card image to power of 2

[Philippe Mathieu-Daudé]

On 7/7/20 5:53 PM, Alistair Francis wrote:
> n Tue, Jul 7, 2020 at 6:21 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> In the next commit 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_truncate() methods and truncate
>> the images of the tests using SD cards.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  tests/acceptance/boot_linux_console.py | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
>> index 3d02519660..f4d4e3635f 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()
>> +
>> +# truncate file size to next power of 2
>> +def image_pow2ceil_truncate(path):
>> +        size = os.path.getsize(path)
>> +        size_aligned = pow2ceil(size)
>> +        if size != size_aligned:
>> +            with open(path, 'ab+') as fd:
>> +                fd.truncate(size_aligned)
> 
> Why truncate the image, can't we expand it instead?

pow2ceil() round UP to the next power of 2. I think this
Python truncate() is a simple wrapper around the ftruncate()
syscall. IOW we only "expand the image". Note, these are
test images copied in tmpdir and discarded after the tests
ran. I'll improve the description.

> 
> Alistair
> 
>> +
>>  class LinuxKernelTest(Test):
>>      KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
>>
>> @@ -635,6 +647,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_truncate(rootfs_path)
>>
>>          self.vm.set_console()
>>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
>> @@ -679,6 +692,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_truncate(image_path)
>>
>>          self.vm.set_console()
>>          self.vm.add_args('-drive', 'file=' + image_path + ',if=sd,format=raw',
>> @@ -728,6 +742,7 @@ 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_pow2ceil_truncate(image_path)
>>          image_drive_args = 'if=sd,format=raw,snapshot=on,file=' + image_path
>>          archive.gzip_uncompress(image_path_gz, image_path)
>>
>> --
>> 2.21.3
>>
>>
> 

Re: [PATCH 1/2] tests/acceptance/boot_linux: Truncate SD card image to power of 2

[Niek Linnenbank]

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

On Tue, Jul 7, 2020 at 3:21 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> In the next commit 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_truncate() methods and truncate
> the images of the tests using SD cards.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> index 3d02519660..f4d4e3635f 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()
> +
> +# truncate file size to next power of 2
> +def image_pow2ceil_truncate(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 '
>
> @@ -635,6 +647,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_truncate(rootfs_path)
>
>          self.vm.set_console()
>          kernel_command_line = (self.KERNEL_COMMON_COMMAND_LINE +
> @@ -679,6 +692,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_truncate(image_path)
>
>          self.vm.set_console()
>          self.vm.add_args('-drive', 'file=' + image_path +
> ',if=sd,format=raw',
> @@ -728,6 +742,7 @@ 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_pow2ceil_truncate(image_path)
>          image_drive_args = 'if=sd,format=raw,snapshot=on,file=' +
> image_path
>          archive.gzip_uncompress(image_path_gz, image_path)
>
> --
> 2.21.3
>
>
Hi Philippe,

This patch works OK for the linux part, but the NetBSD didn't work, it
prints this error:

       (5/5)
tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_arm_orangepi_uboot_netbsd9:
        ERROR: [Errno 2] No such file or directory:
'/var/tmp/avocado_6hoo815w/avocado_job_40aayif8/

5-tests_acceptance_boot_linux_console.py_BootLinuxConsole.test_arm_orangepi_uboot_netbsd9/armv7.img'
(0.18 s)

Basically the truncate should just be moved after the uncompress to fix it.
And the lines that we use before
to extend the image size can be removed now. That was needed to avoid
conflict with the partition size inside image.

So with these small changes, I got it working fine:

diff --git a/tests/acceptance/boot_linux_console.py
b/tests/acceptance/boot_linux_console.py
index f4d4e3635f..69607a5840 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -684,7 +684,7 @@ class BootLinuxConsole(LinuxKernelTest):
         :avocado: tags=machine:orangepi-pc
         """

-        # This test download a 196MB compressed image and expand it to
932MB...
+        # This test download a 196MB compressed image and expand it to 1G
         image_url = ('https://dl.armbian.com/orangepipc/archive/'
                      'Armbian_19.11.3_Orangepipc_bionic_current_5.3.9.7z')
         image_hash = '196a8ffb72b0123d92cea4a070894813d305c71e'
@@ -725,7 +725,7 @@ class BootLinuxConsole(LinuxKernelTest):
         :avocado: tags=arch:arm
         :avocado: tags=machine:orangepi-pc
         """
-        # 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')
@@ -742,9 +742,9 @@ class BootLinuxConsole(LinuxKernelTest):
         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_pow2ceil_truncate(image_path)
         image_drive_args = 'if=sd,format=raw,snapshot=on,file=' +
image_path
         archive.gzip_uncompress(image_path_gz, image_path)
+        image_pow2ceil_truncate(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:
@@ -754,9 +754,9 @@ class BootLinuxConsole(LinuxKernelTest):

                 # 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]))




-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 7116 bytes --]

[PATCH 2/2] 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.

Before CVE-2020-13253 fix, this would allow OOB read/write accesses
past the image size end.

CVE-2020-13253 has been fixed 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 (expecting at least 64 MiB)

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index cb81487e5c..c45106b78e 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"
@@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
+            char *blk_size_aligned_str = size_to_str(blk_size_aligned);
+
+            error_setg(errp, "Invalid SD card size: %s (expecting at least %s)",
+                       blk_size_str, blk_size_aligned_str);
+            g_free(blk_size_str);
+            g_free(blk_size_aligned_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 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Alistair Francis]

On Tue, Jul 7, 2020 at 6:22 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.
>
> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
> past the image size end.
>
> CVE-2020-13253 has been fixed 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 (expecting at least 64 MiB)
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index cb81487e5c..c45106b78e 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"
> @@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
> +            char *blk_size_aligned_str = size_to_str(blk_size_aligned);
> +
> +            error_setg(errp, "Invalid SD card size: %s (expecting at least %s)",
> +                       blk_size_str, blk_size_aligned_str);

Should we print that we expect a power of 2? This isn't always obvious
from the message.

Alistair

> +            g_free(blk_size_str);
> +            g_free(blk_size_aligned_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 2/2] hw/sd/sdcard: Do not allow invalid SD card sizes

[Peter Maydell]

On Tue, 7 Jul 2020 at 17:04, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Jul 7, 2020 at 6:22 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.
> >
> > Before CVE-2020-13253 fix, this would allow OOB read/write accesses
> > past the image size end.
> >
> > CVE-2020-13253 has been fixed 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 (expecting at least 64 MiB)
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  hw/sd/sd.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> > index cb81487e5c..c45106b78e 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"
> > @@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
> > +            char *blk_size_aligned_str = size_to_str(blk_size_aligned);
> > +
> > +            error_setg(errp, "Invalid SD card size: %s (expecting at least %s)",
> > +                       blk_size_str, blk_size_aligned_str);
>
> Should we print that we expect a power of 2? This isn't always obvious
> from the message.

Mmm, I was thinking that. Perhaps
 "expecting a power of 2, e.g. %s"
?

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 7/7/20 6:06 PM, Peter Maydell wrote:
> On Tue, 7 Jul 2020 at 17:04, Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Jul 7, 2020 at 6:22 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.
>>>
>>> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
>>> past the image size end.
>>>
>>> CVE-2020-13253 has been fixed 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 (expecting at least 64 MiB)
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/sd/sd.c | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index cb81487e5c..c45106b78e 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"
>>> @@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
>>> +            char *blk_size_aligned_str = size_to_str(blk_size_aligned);
>>> +
>>> +            error_setg(errp, "Invalid SD card size: %s (expecting at least %s)",
>>> +                       blk_size_str, blk_size_aligned_str);
>>
>> Should we print that we expect a power of 2? This isn't always obvious
>> from the message.
> 
> Mmm, I was thinking that. Perhaps
>  "expecting a power of 2, e.g. %s"
> ?

OK, thanks guys!

> 
> thanks
> -- PMM
> 

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

[Niek Linnenbank]

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

Hi Philippe,

Just tried out your patch on latest master, and I noticed I couldn't apply
it without getting this error:

$ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\
allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\ \<
f4bug@amsat.org\>\ -\ 2020-07-07\ 1521.eml
Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
error: patch failed: hw/sd/sd.c:2130
error: hw/sd/sd.c: patch does not apply
Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

The first patch did go OK. Maybe this one just needs to be rebased, or I
made a mistake.
So I manually copy & pasted the change into hw/sd/sd.c to test it.
It looks like the check works, but my concern is that with this change, we
will be getting this error on 'off-the-shelf' images as well.
For example, the latest Raspbian image size also isn't a power of two:

$ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
WARNING: Image format was not specified for
'/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and probing
guessed raw.
         Automatically detecting the format is dangerous for raw images,
write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)

If we do decide that the change is needed, I would like to propose that we
also give the user some instructions
on how to fix it, maybe some 'dd' command? In my opinion that should also
go in some of the documentation file(s),
possibly also in the one for the OrangePi PC at
docs/system/arm/orangepi.rst (I can also provide a patch for that if you
wish).

Kind regards,

Niek


On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 7/7/20 6:06 PM, Peter Maydell wrote:
> > On Tue, 7 Jul 2020 at 17:04, Alistair Francis <alistair23@gmail.com>
> wrote:
> >>
> >> On Tue, Jul 7, 2020 at 6:22 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.
> >>>
> >>> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
> >>> past the image size end.
> >>>
> >>> CVE-2020-13253 has been fixed 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 (expecting at least 64
> MiB)
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> ---
> >>>  hw/sd/sd.c | 16 ++++++++++++++++
> >>>  1 file changed, 16 insertions(+)
> >>>
> >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >>> index cb81487e5c..c45106b78e 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"
> >>> @@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
> >>> +            char *blk_size_aligned_str =
> size_to_str(blk_size_aligned);
> >>> +
> >>> +            error_setg(errp, "Invalid SD card size: %s (expecting at
> least %s)",
> >>> +                       blk_size_str, blk_size_aligned_str);
> >>
> >> Should we print that we expect a power of 2? This isn't always obvious
> >> from the message.
> >
> > Mmm, I was thinking that. Perhaps
> >  "expecting a power of 2, e.g. %s"
> > ?
>
> OK, thanks guys!
>
> >
> > thanks
> > -- PMM
> >
>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 7644 bytes --]

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

[Philippe Mathieu-Daudé]

On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> Hi Philippe,
> 
> Just tried out your patch on latest master, and I noticed I couldn't
> apply it without getting this error:
> 
> $ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\
> allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\
> \<f4bug@amsat.org <mailto:f4bug@amsat.org>\>\ -\ 2020-07-07\ 1521.eml
> Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
> error: patch failed: hw/sd/sd.c:2130
> error: hw/sd/sd.c: patch does not apply
> Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
> Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
> 
> The first patch did go OK. Maybe this one just needs to be rebased, or I
> made a mistake.

Sorry it was not clear on the cover:

  Part 1 is already reviewed:
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html
  Based-on: <20200630133912.9428-1-f4bug@amsat.org>

This series is based on the "Part 1".

> So I manually copy & pasted the change into hw/sd/sd.c to test it.
> It looks like the check works, but my concern is that with this change,
> we will be getting this error on 'off-the-shelf' images as well.
> For example, the latest Raspbian image size also isn't a power of two:
> 
> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> WARNING: Image format was not specified for
> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> probing guessed raw.
>          Automatically detecting the format is dangerous for raw images,
> write operations on block 0 will be restricted.
>          Specify the 'raw' format explicitly to remove the restrictions.
> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> 
> If we do decide that the change is needed, I would like to propose that
> we also give the user some instructions
> on how to fix it, maybe some 'dd' command?

On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
This is not in the default Darwin packages.
On Windows I have no clue.

> In my opinion that should
> also go in some of the documentation file(s),
> possibly also in the one for the OrangePi PC at
> docs/system/arm/orangepi.rst (I can also provide a patch for that if you
> wish).

Good idea, if you can send that patch that would a precious help,
and I'd include it with the other patches :)

Note that this was your orangepi-pc acceptance test that catched
this bug!
See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672:

 CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=50c5387d
 OF: fdt: Machine model: Xunlong Orange Pi PC
 Kernel command line: printk.time=0 console=ttyS0,115200
root=/dev/mmcblk0 rootwait rw panic=-1 noreboot
 sunxi-mmc 1c0f000.mmc: Linked as a consumer to regulator.2
 sunxi-mmc 1c0f000.mmc: Got CD GPIO
 sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB
 mmc0: host does not support reading read-only switch, assuming write-enable
 mmc0: Problem switching card into high-speed mode!
 mmc0: new SD card at address 4567
 mmcblk0: mmc0:4567 QEMU! 60.0 MiB
 EXT4-fs (mmcblk0): mounting ext2 file system using the ext4 subsystem
 EXT4-fs (mmcblk0): mounted filesystem without journal. Opts: (null)
 VFS: Mounted root (ext2 filesystem) on device 179:0.
 EXT4-fs (mmcblk0): re-mounted. Opts: block_validity,barrier,user_xattr,acl
 Populating /dev using udev: udevd[204]: starting version 3.2.7
 udevadm settle failed
 done
 udevd[205]: worker [208]
/devices/platform/soc/1c0f000.mmc/mmc_host/mmc0/mmc0:4567/block/mmcblk0
is taking a long time
Runner error occurred: Timeout reached
Original status: ERROR

(I'll add that in the commit description too).

Thanks for your testing/review!

> Kind regards,
> 
> Niek
> 
> 
> On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé <f4bug@amsat.org
> <mailto:f4bug@amsat.org>> wrote:
> 
>     On 7/7/20 6:06 PM, Peter Maydell wrote:
>     > On Tue, 7 Jul 2020 at 17:04, Alistair Francis
>     <alistair23@gmail.com <mailto:alistair23@gmail.com>> wrote:
>     >>
>     >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé
>     <f4bug@amsat.org <mailto: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.
>     >>>
>     >>> Before CVE-2020-13253 fix, this would allow OOB read/write accesses
>     >>> past the image size end.
>     >>>
>     >>> CVE-2020-13253 has been fixed 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 (expecting at
>     least 64 MiB)
>     >>>
>     >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>     <mailto:f4bug@amsat.org>>
>     >>> ---
>     >>>  hw/sd/sd.c | 16 ++++++++++++++++
>     >>>  1 file changed, 16 insertions(+)
>     >>>
>     >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>     >>> index cb81487e5c..c45106b78e 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"
>     >>> @@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
>     >>> +            char *blk_size_aligned_str =
>     size_to_str(blk_size_aligned);
>     >>> +
>     >>> +            error_setg(errp, "Invalid SD card size: %s
>     (expecting at least %s)",
>     >>> +                       blk_size_str, blk_size_aligned_str);
>     >>
>     >> Should we print that we expect a power of 2? This isn't always
>     obvious
>     >> from the message.
>     >
>     > Mmm, I was thinking that. Perhaps
>     >  "expecting a power of 2, e.g. %s"
>     > ?
> 
>     OK, thanks guys!
> 
>     >
>     > thanks
>     > -- PMM
>     >
> 
> 
> 
> -- 
> Niek Linnenbank
> 

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

[Peter Maydell]

On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > It looks like the check works, but my concern is that with this change,
> > we will be getting this error on 'off-the-shelf' images as well.
> > For example, the latest Raspbian image size also isn't a power of two:
> >
> > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > WARNING: Image format was not specified for
> > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > probing guessed raw.
> >          Automatically detecting the format is dangerous for raw images,
> > write operations on block 0 will be restricted.
> >          Specify the 'raw' format explicitly to remove the restrictions.
> > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> >
> > If we do decide that the change is needed, I would like to propose that
> > we also give the user some instructions
> > on how to fix it, maybe some 'dd' command?
>
> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> This is not in the default Darwin packages.
> On Windows I have no clue.

dd/truncate etc won't work if the image file is not raw (eg if
it's qcow2). The only chance you have of something that's actually
generic would probably involve "qemu-img resize". But I'm a bit
wary of having an error message that recommends that, because
what if we got it wrong?

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 7/9/20 4:15 PM, Peter Maydell wrote:
> On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
>>> So I manually copy & pasted the change into hw/sd/sd.c to test it.
>>> It looks like the check works, but my concern is that with this change,
>>> we will be getting this error on 'off-the-shelf' images as well.
>>> For example, the latest Raspbian image size also isn't a power of two:
>>>
>>> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
>>> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
>>> WARNING: Image format was not specified for
>>> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
>>> probing guessed raw.
>>>          Automatically detecting the format is dangerous for raw images,
>>> write operations on block 0 will be restricted.
>>>          Specify the 'raw' format explicitly to remove the restrictions.
>>> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
>>>
>>> If we do decide that the change is needed, I would like to propose that
>>> we also give the user some instructions
>>> on how to fix it, maybe some 'dd' command?
>>
>> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
>> This is not in the default Darwin packages.
>> On Windows I have no clue.
> 
> dd/truncate etc won't work if the image file is not raw (eg if
> it's qcow2).

Good catch...

> The only chance you have of something that's actually
> generic would probably involve "qemu-img resize". But I'm a bit
> wary of having an error message that recommends that, because
> what if we got it wrong?

I am not sure what to recommend then.

Would that work as hint?

  qemu-system-arm -M raspi2 -sd ./buster-lite-armhf.img
  qemu-system-arm: Invalid SD card size: 1.73 GiB
  SD card size has to be a power of 2, e.g. 2GiB.

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

[Alistair Francis]

On Thu, Jul 9, 2020 at 7:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 7/9/20 4:15 PM, Peter Maydell wrote:
> > On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> >>> So I manually copy & pasted the change into hw/sd/sd.c to test it.
> >>> It looks like the check works, but my concern is that with this change,
> >>> we will be getting this error on 'off-the-shelf' images as well.
> >>> For example, the latest Raspbian image size also isn't a power of two:
> >>>
> >>> $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> >>> ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> >>> WARNING: Image format was not specified for
> >>> '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> >>> probing guessed raw.
> >>>          Automatically detecting the format is dangerous for raw images,
> >>> write operations on block 0 will be restricted.
> >>>          Specify the 'raw' format explicitly to remove the restrictions.
> >>> qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> >>>
> >>> If we do decide that the change is needed, I would like to propose that
> >>> we also give the user some instructions
> >>> on how to fix it, maybe some 'dd' command?
> >>
> >> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> >> This is not in the default Darwin packages.
> >> On Windows I have no clue.
> >
> > dd/truncate etc won't work if the image file is not raw (eg if
> > it's qcow2).
>
> Good catch...
>
> > The only chance you have of something that's actually
> > generic would probably involve "qemu-img resize". But I'm a bit
> > wary of having an error message that recommends that, because
> > what if we got it wrong?
>
> I am not sure what to recommend then.
>
> Would that work as hint?
>
>   qemu-system-arm -M raspi2 -sd ./buster-lite-armhf.img
>   qemu-system-arm: Invalid SD card size: 1.73 GiB
>   SD card size has to be a power of 2, e.g. 2GiB.

That sounds good to me. That's enough for a user to figure out the next step.

If you want you could also add: "qemu-img might be able to help." or
something like that.

Alistair

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

[Peter Maydell]

On Thu, 9 Jul 2020 at 17:27, Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Jul 9, 2020 at 7:35 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 7/9/20 4:15 PM, Peter Maydell wrote:
> > > The only chance you have of something that's actually
> > > generic would probably involve "qemu-img resize". But I'm a bit
> > > wary of having an error message that recommends that, because
> > > what if we got it wrong?
> >
> > I am not sure what to recommend then.
> >
> > Would that work as hint?
> >
> >   qemu-system-arm -M raspi2 -sd ./buster-lite-armhf.img
> >   qemu-system-arm: Invalid SD card size: 1.73 GiB
> >   SD card size has to be a power of 2, e.g. 2GiB.
>
> That sounds good to me. That's enough for a user to figure out the next step.
>
> If you want you could also add: "qemu-img might be able to help." or
> something like that.

Thinking about it a bit and talking to Philippe on IRC, I think
we can usefully have the message recommend "qemu-img resize" to the
user; I think we should avoid printing out an exact-cut-n-paste
command line just in case it's wrong, but something like:
 qemu-system-arm: Invalid SD card size: 1.73 GiB
 SD card size has to be a power of 2, e.g. 2GiB.
 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)

I think is a reasonable balance between pointing the user in
the right direction and cautioning them to check what they're
doing before they blithely perform the resize...

thanks
-- PMM

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

[Niek Linnenbank]

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

On Thu, Jul 9, 2020 at 4:15 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> >
> > On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > > It looks like the check works, but my concern is that with this change,
> > > we will be getting this error on 'off-the-shelf' images as well.
> > > For example, the latest Raspbian image size also isn't a power of two:
> > >
> > > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > > WARNING: Image format was not specified for
> > > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > > probing guessed raw.
> > >          Automatically detecting the format is dangerous for raw
> images,
> > > write operations on block 0 will be restricted.
> > >          Specify the 'raw' format explicitly to remove the
> restrictions.
> > > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2
> GiB)
> > >
> > > If we do decide that the change is needed, I would like to propose that
> > > we also give the user some instructions
> > > on how to fix it, maybe some 'dd' command?
> >
> > On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> > This is not in the default Darwin packages.
> > On Windows I have no clue.
>
> dd/truncate etc won't work if the image file is not raw (eg if
> it's qcow2). The only chance you have of something that's actually
> generic would probably involve "qemu-img resize". But I'm a bit
> wary of having an error message that recommends that, because
> what if we got it wrong?
>

Yeah good point Peter, I see what you mean. As I wrote to Philippe,
i'll try to make a small patch with some instructions in the OrangePi board
documentation,
so then we'll at least have something there to help the user.

Regards,
Niek


>
> thanks
> -- PMM
>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 3008 bytes --]

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

[Kevin Wolf]

Am 09.07.2020 um 16:15 hat Peter Maydell geschrieben:
> On Thu, 9 Jul 2020 at 14:56, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > > It looks like the check works, but my concern is that with this change,
> > > we will be getting this error on 'off-the-shelf' images as well.
> > > For example, the latest Raspbian image size also isn't a power of two:
> > >
> > > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > > WARNING: Image format was not specified for
> > > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > > probing guessed raw.
> > >          Automatically detecting the format is dangerous for raw images,
> > > write operations on block 0 will be restricted.
> > >          Specify the 'raw' format explicitly to remove the restrictions.
> > > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2 GiB)
> > >
> > > If we do decide that the change is needed, I would like to propose that
> > > we also give the user some instructions
> > > on how to fix it, maybe some 'dd' command?
> >
> > On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> > This is not in the default Darwin packages.
> > On Windows I have no clue.
> 
> dd/truncate etc won't work if the image file is not raw (eg if
> it's qcow2). The only chance you have of something that's actually
> generic would probably involve "qemu-img resize". But I'm a bit
> wary of having an error message that recommends that, because
> what if we got it wrong?

What is your concern that we might get wrong? The suggestion is always
extending the size rather than shrinking, so it should be harmless and
easy to undo. (Hm, we should finally make --shrink mandatory for
shrinking. We've printed a deprecation warning for almost three years.)

Kevin

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

[Peter Maydell]

On Fri, 10 Jul 2020 at 10:58, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 09.07.2020 um 16:15 hat Peter Maydell geschrieben:
> > dd/truncate etc won't work if the image file is not raw (eg if
> > it's qcow2). The only chance you have of something that's actually
> > generic would probably involve "qemu-img resize". But I'm a bit
> > wary of having an error message that recommends that, because
> > what if we got it wrong?
>
> What is your concern that we might get wrong? The suggestion is always
> extending the size rather than shrinking, so it should be harmless and
> easy to undo. (Hm, we should finally make --shrink mandatory for
> shrinking. We've printed a deprecation warning for almost three years.)

If there's a qemu-img command line that will always only
extend the image size and never let the user accidentally
shrink it and throw away data, then great. I'd happily
recommend that.

thanks
-- PMM

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

[Kevin Wolf]

Am 10.07.2020 um 11:59 hat Peter Maydell geschrieben:
> On Fri, 10 Jul 2020 at 10:58, Kevin Wolf <kwolf@redhat.com> wrote:
> >
> > Am 09.07.2020 um 16:15 hat Peter Maydell geschrieben:
> > > dd/truncate etc won't work if the image file is not raw (eg if
> > > it's qcow2). The only chance you have of something that's actually
> > > generic would probably involve "qemu-img resize". But I'm a bit
> > > wary of having an error message that recommends that, because
> > > what if we got it wrong?
> >
> > What is your concern that we might get wrong? The suggestion is always
> > extending the size rather than shrinking, so it should be harmless and
> > easy to undo. (Hm, we should finally make --shrink mandatory for
> > shrinking. We've printed a deprecation warning for almost three years.)
> 
> If there's a qemu-img command line that will always only
> extend the image size and never let the user accidentally
> shrink it and throw away data, then great. I'd happily
> recommend that.

I think removing deprecated behaviour is a change that we can still make
in the early freeze. So if you agree, I'll send a patch that makes
shrinking an image without --shrink a hard error in 5.1.

Kevin

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

[Peter Maydell]

On Fri, 10 Jul 2020 at 13:07, Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 10.07.2020 um 11:59 hat Peter Maydell geschrieben:
> > On Fri, 10 Jul 2020 at 10:58, Kevin Wolf <kwolf@redhat.com> wrote:
> > >
> > > Am 09.07.2020 um 16:15 hat Peter Maydell geschrieben:
> > > > dd/truncate etc won't work if the image file is not raw (eg if
> > > > it's qcow2). The only chance you have of something that's actually
> > > > generic would probably involve "qemu-img resize". But I'm a bit
> > > > wary of having an error message that recommends that, because
> > > > what if we got it wrong?
> > >
> > > What is your concern that we might get wrong? The suggestion is always
> > > extending the size rather than shrinking, so it should be harmless and
> > > easy to undo. (Hm, we should finally make --shrink mandatory for
> > > shrinking. We've printed a deprecation warning for almost three years.)
> >
> > If there's a qemu-img command line that will always only
> > extend the image size and never let the user accidentally
> > shrink it and throw away data, then great. I'd happily
> > recommend that.
>
> I think removing deprecated behaviour is a change that we can still make
> in the early freeze. So if you agree, I'll send a patch that makes
> shrinking an image without --shrink a hard error in 5.1.

Happy to defer to your judgement on that; I agree that removal
of deprecated behaviour is ok at this point in freeze.

-- PMM

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

[Niek Linnenbank]

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

On Thu, Jul 9, 2020 at 3:56 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> On 7/7/20 10:29 PM, Niek Linnenbank wrote:
> > Hi Philippe,
> >
> > Just tried out your patch on latest master, and I noticed I couldn't
> > apply it without getting this error:
> >
> > $ git am ~/Downloads/patches/\[PATCH\ 2_2\]\ hw_sd_sdcard\:\ Do\ not\
> > allow\ invalid\ SD\ card\ sizes\ -\ Philippe\ Mathieu-Daudé\
> > \<f4bug@amsat.org <mailto:f4bug@amsat.org>\>\ -\ 2020-07-07\ 1521.eml
> > Applying: hw/sd/sdcard: Do not allow invalid SD card sizes
> > error: patch failed: hw/sd/sd.c:2130
> > error: hw/sd/sd.c: patch does not apply
> > Patch failed at 0001 hw/sd/sdcard: Do not allow invalid SD card sizes
> > Use 'git am --show-current-patch' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> >
> > The first patch did go OK. Maybe this one just needs to be rebased, or I
> > made a mistake.
>
> Sorry it was not clear on the cover:
>
>   Part 1 is already reviewed:
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg718150.html
>   Based-on: <20200630133912.9428-1-f4bug@amsat.org>
>
> This series is based on the "Part 1".
>
> > So I manually copy & pasted the change into hw/sd/sd.c to test it.
> > It looks like the check works, but my concern is that with this change,
> > we will be getting this error on 'off-the-shelf' images as well.
> > For example, the latest Raspbian image size also isn't a power of two:
> >
> > $ ./arm-softmmu/qemu-system-arm -M raspi2 -sd
> > ~/Downloads/2020-05-27-raspios-buster-lite-armhf.img -nographic
> > WARNING: Image format was not specified for
> > '/home/me/Downloads/2020-05-27-raspios-buster-lite-armhf.img' and
> > probing guessed raw.
> >          Automatically detecting the format is dangerous for raw images,
> > write operations on block 0 will be restricted.
> >          Specify the 'raw' format explicitly to remove the restrictions.
> > qemu-system-arm: Invalid SD card size: 1.73 GiB (expecting at least 2
> GiB)
> >
> > If we do decide that the change is needed, I would like to propose that
> > we also give the user some instructions
> > on how to fix it, maybe some 'dd' command?
>
> On POSIX we can suggest to use 'truncate -s 2G' from coreutils.
> This is not in the default Darwin packages.
> On Windows I have no clue.
>
> > In my opinion that should
> > also go in some of the documentation file(s),
> > possibly also in the one for the OrangePi PC at
> > docs/system/arm/orangepi.rst (I can also provide a patch for that if you
> > wish).
>
> Good idea, if you can send that patch that would a precious help,
> and I'd include it with the other patches :)
>

OK Philipe. Then I'll prepare a patch and try send it to the list somewhere
this weekend.


>
> Note that this was your orangepi-pc acceptance test that catched
> this bug!
> See https://travis-ci.org/github/philmd/qemu/jobs/705653532#L5672:
>
>
Oh cool, that is great. Looks like it is working pretty well then. But lets
be fair, I think it was you that contributed that part ;-)


>  CPU: ARMv7 Processor [410fc075] revision 5 (ARMv7), cr=50c5387d
>  OF: fdt: Machine model: Xunlong Orange Pi PC
>  Kernel command line: printk.time=0 console=ttyS0,115200
> root=/dev/mmcblk0 rootwait rw panic=-1 noreboot
>  sunxi-mmc 1c0f000.mmc: Linked as a consumer to regulator.2
>  sunxi-mmc 1c0f000.mmc: Got CD GPIO
>  sunxi-mmc 1c0f000.mmc: initialized, max. request size: 16384 KB
>  mmc0: host does not support reading read-only switch, assuming
> write-enable
>  mmc0: Problem switching card into high-speed mode!
>  mmc0: new SD card at address 4567
>  mmcblk0: mmc0:4567 QEMU! 60.0 MiB
>  EXT4-fs (mmcblk0): mounting ext2 file system using the ext4 subsystem
>  EXT4-fs (mmcblk0): mounted filesystem without journal. Opts: (null)
>  VFS: Mounted root (ext2 filesystem) on device 179:0.
>  EXT4-fs (mmcblk0): re-mounted. Opts: block_validity,barrier,user_xattr,acl
>  Populating /dev using udev: udevd[204]: starting version 3.2.7
>  udevadm settle failed
>  done
>  udevd[205]: worker [208]
> /devices/platform/soc/1c0f000.mmc/mmc_host/mmc0/mmc0:4567/block/mmcblk0
> is taking a long time
> Runner error occurred: Timeout reached
> Original status: ERROR
>
> (I'll add that in the commit description too).
>

OK thanks!

>
> Thanks for your testing/review!
>
> > Kind regards,
> >
> > Niek
> >
> >
> > On Tue, Jul 7, 2020 at 6:11 PM Philippe Mathieu-Daudé <f4bug@amsat.org
> > <mailto:f4bug@amsat.org>> wrote:
> >
> >     On 7/7/20 6:06 PM, Peter Maydell wrote:
> >     > On Tue, 7 Jul 2020 at 17:04, Alistair Francis
> >     <alistair23@gmail.com <mailto:alistair23@gmail.com>> wrote:
> >     >>
> >     >> On Tue, Jul 7, 2020 at 6:22 AM Philippe Mathieu-Daudé
> >     <f4bug@amsat.org <mailto: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.
> >     >>>
> >     >>> Before CVE-2020-13253 fix, this would allow OOB read/write
> accesses
> >     >>> past the image size end.
> >     >>>
> >     >>> CVE-2020-13253 has been fixed 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 (expecting at
> >     least 64 MiB)
> >     >>>
> >     >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
> >     <mailto:f4bug@amsat.org>>
> >     >>> ---
> >     >>>  hw/sd/sd.c | 16 ++++++++++++++++
> >     >>>  1 file changed, 16 insertions(+)
> >     >>>
> >     >>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> >     >>> index cb81487e5c..c45106b78e 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"
> >     >>> @@ -2130,11 +2131,26 @@ 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 = size_to_str(blk_size);
> >     >>> +            char *blk_size_aligned_str =
> >     size_to_str(blk_size_aligned);
> >     >>> +
> >     >>> +            error_setg(errp, "Invalid SD card size: %s
> >     (expecting at least %s)",
> >     >>> +                       blk_size_str, blk_size_aligned_str);
> >     >>
> >     >> Should we print that we expect a power of 2? This isn't always
> >     obvious
> >     >> from the message.
> >     >
> >     > Mmm, I was thinking that. Perhaps
> >     >  "expecting a power of 2, e.g. %s"
> >     > ?
> >
> >     OK, thanks guys!
> >
> >     >
> >     > thanks
> >     > -- PMM
> >     >
> >
> >
> >
> > --
> > Niek Linnenbank
> >
>


-- 
Niek Linnenbank

[-- Attachment #2: Type: text/html, Size: 13022 bytes --]