[RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again

[RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again

[Philippe Mathieu-Daudé]

Hi Ubi-Wan Kenubi and Tom,

In commit a9bcedd (SD card size has to be power of 2) we decided
to restrict SD card size to avoid security problems (CVE-2020-13253)
but this became not practical to some users.

This RFC series tries to remove the limitation, keeping our
functional tests working. It is unfinished work because I had to
attend other topics, but sending it early as RFC to get feedback.
I'll keep working when I get more time, except if one if you can
help me.

Alexander, could you generate a qtest reproducer with the fuzzer
corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054

Thanks,

Phil.

Philippe Mathieu-Daudé (9):
  hw/sd: When card is in wrong state, log which state it is
  hw/sd: Extract address_in_range() helper, log invalid accesses
  tests/acceptance: Tag NetBSD tests as 'os:netbsd'
  tests/acceptance: Extract image_expand() helper
  tests/acceptance: Use image_expand() in
    test_arm_orangepi_uboot_netbsd9
  tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08
  tests/acceptance: Do not expand SD card image in test_arm_orangepi_sd
  tests/acceptance: Remove now unused pow2ceil()
  hw/sd: Allow card size not power of 2 again

 hw/sd/sd.c                             | 60 +++++++++++++-------------
 tests/acceptance/boot_linux_console.py | 39 ++++++++---------
 tests/acceptance/ppc_prep_40p.py       |  2 +
 3 files changed, 52 insertions(+), 49 deletions(-)

-- 
2.31.1

[PATCH 1/9] hw/sd: When card is in wrong state, log which state it is

[Philippe Mathieu-Daudé]

We report the card is in an inconsistent state, but don't precise
in which state it is. Add this information, as it is useful when
debugging problems.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 282d39a7042..d8fdf84f4db 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1504,7 +1504,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         return sd_illegal;
     }
 
-    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd);
+    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
+                  req.cmd, sd_state_name(sd->state));
     return sd_illegal;
 }
 
-- 
2.31.1

Re: [PATCH 1/9] hw/sd: When card is in wrong state, log which state it is

[Eric Blake]

On Wed, Jun 23, 2021 at 08:00:13PM +0200, Philippe Mathieu-Daudé wrote:
> We report the card is in an inconsistent state, but don't precise

s/don't/aren't/

> in which state it is. Add this information, as it is useful when
> debugging problems.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 282d39a7042..d8fdf84f4db 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1504,7 +1504,8 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
>          return sd_illegal;
>      }
>  
> -    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state\n", req.cmd);
> +    qemu_log_mask(LOG_GUEST_ERROR, "SD: CMD%i in a wrong state: %s\n",
> +                  req.cmd, sd_state_name(sd->state));
>      return sd_illegal;
>  }
>  
> -- 
> 2.31.1
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH 2/9] hw/sd: Extract address_in_range() helper, log invalid accesses

[Philippe Mathieu-Daudé]

Multiple commands have to check the address requested is valid.
Extract this code pattern as a new address_in_range() helper, and
log invalid accesses as guest errors.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index d8fdf84f4db..9c8dd11bad1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -937,6 +937,18 @@ static void sd_lock_command(SDState *sd)
         sd->card_status &= ~CARD_IS_LOCKED;
 }
 
+static bool address_in_range(SDState *sd, const char *desc,
+                             uint64_t addr, uint32_t length)
+{
+    if (addr + length > sd->size) {
+        qemu_log_mask(LOG_GUEST_ERROR, "%s offset %lu > card %lu [%%%u]\n",
+                      desc, addr, sd->size, length);
+        sd->card_status |= ADDRESS_ERROR;
+        return false;
+    }
+    return true;
+}
+
 static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 {
     uint32_t rca = 0x0000;
@@ -1218,8 +1230,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         switch (sd->state) {
         case sd_transfer_state:
 
-            if (addr + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "READ_BLOCK", addr, sd->blk_len)) {
                 return sd_r1;
             }
 
@@ -1264,8 +1275,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         switch (sd->state) {
         case sd_transfer_state:
 
-            if (addr + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "WRITE_BLOCK", addr, sd->blk_len)) {
                 return sd_r1;
             }
 
@@ -1325,8 +1335,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
         switch (sd->state) {
         case sd_transfer_state:
-            if (addr >= sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "SET_WRITE_PROT", addr, 1)) {
                 return sd_r1b;
             }
 
@@ -1348,8 +1357,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
 
         switch (sd->state) {
         case sd_transfer_state:
-            if (addr >= sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "CLR_WRITE_PROT", addr, 1)) {
                 return sd_r1b;
             }
 
@@ -1826,8 +1834,8 @@ void sd_write_byte(SDState *sd, uint8_t value)
     case 25:	/* CMD25:  WRITE_MULTIPLE_BLOCK */
         if (sd->data_offset == 0) {
             /* Start of the block - let's check the address is valid */
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "WRITE_MULTIPLE_BLOCK",
+                                  sd->data_start, sd->blk_len)) {
                 break;
             }
             if (sd->size <= SDSC_MAX_CAPACITY) {
@@ -1999,8 +2007,8 @@ uint8_t sd_read_byte(SDState *sd)
 
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
         if (sd->data_offset == 0) {
-            if (sd->data_start + io_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
+            if (!address_in_range(sd, "READ_MULTIPLE_BLOCK",
+                                  sd->data_start, io_len)) {
                 return 0x00;
             }
             BLK_READ_BLOCK(sd->data_start, io_len);
-- 
2.31.1

Re: [PATCH 2/9] hw/sd: Extract address_in_range() helper, log invalid accesses

[Eric Blake]

On Wed, Jun 23, 2021 at 08:00:14PM +0200, Philippe Mathieu-Daudé wrote:
> Multiple commands have to check the address requested is valid.

check that the

> Extract this code pattern as a new address_in_range() helper, and
> log invalid accesses as guest errors.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index d8fdf84f4db..9c8dd11bad1 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -937,6 +937,18 @@ static void sd_lock_command(SDState *sd)
>          sd->card_status &= ~CARD_IS_LOCKED;
>  }
>  
> +static bool address_in_range(SDState *sd, const char *desc,
> +                             uint64_t addr, uint32_t length)
> +{
> +    if (addr + length > sd->size) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s offset %lu > card %lu [%%%u]\n",
> +                      desc, addr, sd->size, length);

For a (fictitiously small) device with 2048 bytes and a read request
of 2k at offset 1k, this results in the odd message:

READ_BLOCK offset 1024 > card 2048 [%2048]

Would it be any better as:

"%s offset+length %lu+%lu > card size %lu\n"

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Philippe Mathieu-Daudé]

Avocado allows us to select set of tests using tags.
When wanting to run all tests using a NetBSD guest OS,
it is convenient to have them tagged, add the 'os:netbsd'
tag.

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

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index cded547d1d4..20d57c1a8c6 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -862,6 +862,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
         :avocado: tags=arch:arm
         :avocado: tags=machine:orangepi-pc
         :avocado: tags=device:sd
+        :avocado: tags=os:netbsd
         """
         # This test download a 304MB compressed image and expand it to 2GB
         deb_url = ('http://snapshot.debian.org/archive/debian/'
diff --git a/tests/acceptance/ppc_prep_40p.py b/tests/acceptance/ppc_prep_40p.py
index 96ba13b8943..2993ee3b078 100644
--- a/tests/acceptance/ppc_prep_40p.py
+++ b/tests/acceptance/ppc_prep_40p.py
@@ -27,6 +27,7 @@ def test_factory_firmware_and_netbsd(self):
         """
         :avocado: tags=arch:ppc
         :avocado: tags=machine:40p
+        :avocado: tags=os:netbsd
         :avocado: tags=slowness:high
         """
         bios_url = ('http://ftpmirror.your.org/pub/misc/'
@@ -64,6 +65,7 @@ def test_openbios_and_netbsd(self):
         """
         :avocado: tags=arch:ppc
         :avocado: tags=machine:40p
+        :avocado: tags=os:netbsd
         """
         drive_url = ('https://cdn.netbsd.org/pub/NetBSD/iso/7.1.2/'
                      'NetBSD-7.1.2-prep.iso')
-- 
2.31.1

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Philippe Mathieu-Daudé]

CC'ing NetBSD maintainers.

On 6/23/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> Avocado allows us to select set of tests using tags.
> When wanting to run all tests using a NetBSD guest OS,
> it is convenient to have them tagged, add the 'os:netbsd'
> tag.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 1 +
>  tests/acceptance/ppc_prep_40p.py       | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index cded547d1d4..20d57c1a8c6 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -862,6 +862,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
>          :avocado: tags=arch:arm
>          :avocado: tags=machine:orangepi-pc
>          :avocado: tags=device:sd
> +        :avocado: tags=os:netbsd
>          """
>          # This test download a 304MB compressed image and expand it to 2GB
>          deb_url = ('http://snapshot.debian.org/archive/debian/'
> diff --git a/tests/acceptance/ppc_prep_40p.py b/tests/acceptance/ppc_prep_40p.py
> index 96ba13b8943..2993ee3b078 100644
> --- a/tests/acceptance/ppc_prep_40p.py
> +++ b/tests/acceptance/ppc_prep_40p.py
> @@ -27,6 +27,7 @@ def test_factory_firmware_and_netbsd(self):
>          """
>          :avocado: tags=arch:ppc
>          :avocado: tags=machine:40p
> +        :avocado: tags=os:netbsd
>          :avocado: tags=slowness:high
>          """
>          bios_url = ('http://ftpmirror.your.org/pub/misc/'
> @@ -64,6 +65,7 @@ def test_openbios_and_netbsd(self):
>          """
>          :avocado: tags=arch:ppc
>          :avocado: tags=machine:40p
> +        :avocado: tags=os:netbsd
>          """
>          drive_url = ('https://cdn.netbsd.org/pub/NetBSD/iso/7.1.2/'
>                       'NetBSD-7.1.2-prep.iso')
> 

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Philippe Mathieu-Daudé]

On Sat, Jul 3, 2021 at 10:41 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> CC'ing NetBSD maintainers.
>
> On 6/23/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> > Avocado allows us to select set of tests using tags.
> > When wanting to run all tests using a NetBSD guest OS,
> > it is convenient to have them tagged, add the 'os:netbsd'
> > tag.

I'll amend an command line example to run the NetBSD tests:

   $ avocado --show=app,console run -t os:netbsd tests/acceptance/

> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  tests/acceptance/boot_linux_console.py | 1 +
> >  tests/acceptance/ppc_prep_40p.py       | 2 ++
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> > index cded547d1d4..20d57c1a8c6 100644
> > --- a/tests/acceptance/boot_linux_console.py
> > +++ b/tests/acceptance/boot_linux_console.py
> > @@ -862,6 +862,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
> >          :avocado: tags=arch:arm
> >          :avocado: tags=machine:orangepi-pc
> >          :avocado: tags=device:sd
> > +        :avocado: tags=os:netbsd
> >          """
> >          # This test download a 304MB compressed image and expand it to 2GB
> >          deb_url = ('http://snapshot.debian.org/archive/debian/'
> > diff --git a/tests/acceptance/ppc_prep_40p.py b/tests/acceptance/ppc_prep_40p.py
> > index 96ba13b8943..2993ee3b078 100644
> > --- a/tests/acceptance/ppc_prep_40p.py
> > +++ b/tests/acceptance/ppc_prep_40p.py
> > @@ -27,6 +27,7 @@ def test_factory_firmware_and_netbsd(self):
> >          """
> >          :avocado: tags=arch:ppc
> >          :avocado: tags=machine:40p
> > +        :avocado: tags=os:netbsd
> >          :avocado: tags=slowness:high
> >          """
> >          bios_url = ('http://ftpmirror.your.org/pub/misc/'
> > @@ -64,6 +65,7 @@ def test_openbios_and_netbsd(self):
> >          """
> >          :avocado: tags=arch:ppc
> >          :avocado: tags=machine:40p
> > +        :avocado: tags=os:netbsd
> >          """
> >          drive_url = ('https://cdn.netbsd.org/pub/NetBSD/iso/7.1.2/'
> >                       'NetBSD-7.1.2-prep.iso')
> >
>

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Niek Linnenbank]

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

for test_arm_orangepi_uboot_netbsd9:

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Op za 3 jul. 2021 10:44 schreef Philippe Mathieu-Daudé <f4bug@amsat.org>:

> On Sat, Jul 3, 2021 at 10:41 AM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> >
> > CC'ing NetBSD maintainers.
> >
> > On 6/23/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> > > Avocado allows us to select set of tests using tags.
> > > When wanting to run all tests using a NetBSD guest OS,
> > > it is convenient to have them tagged, add the 'os:netbsd'
> > > tag.
>
> I'll amend an command line example to run the NetBSD tests:
>
>    $ avocado --show=app,console run -t os:netbsd tests/acceptance/
>
> > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > >  tests/acceptance/boot_linux_console.py | 1 +
> > >  tests/acceptance/ppc_prep_40p.py       | 2 ++
> > >  2 files changed, 3 insertions(+)
> > >
> > > diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> > > index cded547d1d4..20d57c1a8c6 100644
> > > --- a/tests/acceptance/boot_linux_console.py
> > > +++ b/tests/acceptance/boot_linux_console.py
> > > @@ -862,6 +862,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
> > >          :avocado: tags=arch:arm
> > >          :avocado: tags=machine:orangepi-pc
> > >          :avocado: tags=device:sd
> > > +        :avocado: tags=os:netbsd
> > >          """
> > >          # This test download a 304MB compressed image and expand it
> to 2GB
> > >          deb_url = ('http://snapshot.debian.org/archive/debian/'
> > > diff --git a/tests/acceptance/ppc_prep_40p.py
> b/tests/acceptance/ppc_prep_40p.py
> > > index 96ba13b8943..2993ee3b078 100644
> > > --- a/tests/acceptance/ppc_prep_40p.py
> > > +++ b/tests/acceptance/ppc_prep_40p.py
> > > @@ -27,6 +27,7 @@ def test_factory_firmware_and_netbsd(self):
> > >          """
> > >          :avocado: tags=arch:ppc
> > >          :avocado: tags=machine:40p
> > > +        :avocado: tags=os:netbsd
> > >          :avocado: tags=slowness:high
> > >          """
> > >          bios_url = ('http://ftpmirror.your.org/pub/misc/'
> > > @@ -64,6 +65,7 @@ def test_openbios_and_netbsd(self):
> > >          """
> > >          :avocado: tags=arch:ppc
> > >          :avocado: tags=machine:40p
> > > +        :avocado: tags=os:netbsd
> > >          """
> > >          drive_url = ('https://cdn.netbsd.org/pub/NetBSD/iso/7.1.2/'
> > >                       'NetBSD-7.1.2-prep.iso')
> > >
> >
>

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

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Philippe Mathieu-Daudé]

Hi Niek,

On 7/4/21 2:35 PM, Niek Linnenbank wrote:
> for test_arm_orangepi_uboot_netbsd9:
> 
> Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com
> <mailto:nieklinnenbank@gmail.com>>

Thanks for the review. Does your R-b tag applies for this single
patch or all patches related to test_arm_orangepi_uboot_netbsd9
in this series (3-5)?

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Philippe Mathieu-Daudé]

On 7/5/21 10:55 AM, Philippe Mathieu-Daudé wrote:
> Hi Niek,
> 
> On 7/4/21 2:35 PM, Niek Linnenbank wrote:
>> for test_arm_orangepi_uboot_netbsd9:
>>
>> Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com
>> <mailto:nieklinnenbank@gmail.com>>
> 
> Thanks for the review. Does your R-b tag applies for this single
> patch or all patches related to test_arm_orangepi_uboot_netbsd9
> in this series (3-5)?

Soft-freeze is in 2 days and no review from the NetBSD team,
so postponing these patches to v6.2.

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:03 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Avocado allows us to select set of tests using tags.
> When wanting to run all tests using a NetBSD guest OS,
> it is convenient to have them tagged, add the 'os:netbsd'
> tag.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 1 +
>  tests/acceptance/ppc_prep_40p.py       | 2 ++
>  2 files changed, 3 insertions(+)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 3/9] tests/acceptance: Tag NetBSD tests as 'os:netbsd'

[Cleber Rosa]

Philippe Mathieu-Daudé writes:

> Avocado allows us to select set of tests using tags.
> When wanting to run all tests using a NetBSD guest OS,
> it is convenient to have them tagged, add the 'os:netbsd'
> tag.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 1 +
>  tests/acceptance/ppc_prep_40p.py       | 2 ++
>  2 files changed, 3 insertions(+)

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

Phil,

I can ammend the commit message and queue this one if you think it's a
good idea.

Cheers,
- Cleber.

[PATCH 4/9] tests/acceptance: Extract image_expand() helper

[Philippe Mathieu-Daudé]

To be able to expand an image to a non-power-of-2 value,
extract image_expand() from image_pow2ceil_expand().

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

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 20d57c1a8c6..61069f0064f 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -35,15 +35,19 @@
 def pow2ceil(x):
     return 1 if x == 0 else 2**(x - 1).bit_length()
 
+"""
+Expand file size
+"""
+def image_expand(path, size):
+    if size != os.path.getsize(path):
+        with open(path, 'ab+') as fd:
+            fd.truncate(size)
+
 """
 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)
+    image_expand(path, pow2ceil(os.path.getsize(path)))
 
 class LinuxKernelTest(Test):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
-- 
2.31.1

Re: [PATCH 4/9] tests/acceptance: Extract image_expand() helper

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> To be able to expand an image to a non-power-of-2 value,
> extract image_expand() from image_pow2ceil_expand().
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 4/9] tests/acceptance: Extract image_expand() helper

[Cleber Rosa]

Philippe Mathieu-Daudé writes:

> To be able to expand an image to a non-power-of-2 value,
> extract image_expand() from image_pow2ceil_expand().
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 20d57c1a8c6..61069f0064f 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -35,15 +35,19 @@
>  def pow2ceil(x):
>      return 1 if x == 0 else 2**(x - 1).bit_length()
>  
> +"""
> +Expand file size
> +"""
> +def image_expand(path, size):
> +    if size != os.path.getsize(path):
> +        with open(path, 'ab+') as fd:
> +            fd.truncate(size)
> +

This would be a good time to make the comment section, into a proper
docstring, that is:

def image_expand(path, size):
   """Expand file size"""
    if size != os.path.getsize(path):
        with open(path, 'ab+') as fd:
            fd.truncate(size)

- Cleber.

[PATCH 5/9] tests/acceptance: Use image_expand() in test_arm_orangepi_uboot_netbsd9

[Philippe Mathieu-Daudé]

The NetBSD OrangePi image must be at least 2GiB, not less.
Expand the SD card image to this size before using it.

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

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 61069f0064f..b10f7257503 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -868,7 +868,12 @@ def test_arm_orangepi_uboot_netbsd9(self):
         :avocado: tags=device:sd
         :avocado: tags=os:netbsd
         """
-        # This test download a 304MB compressed image and expand it to 2GB
+        # This test download a 304MB compressed image and expand it to 2GB,
+        # which is the minimum card size required by the NetBSD installer:
+        # https://wiki.netbsd.org/ports/evbarm/raspberry_pi/#index7h2
+        # "A 2 GB card is the smallest workable size that the installation
+        # image will fit on."
+        NETBSD_SDCARD_MINSIZE = 2 * 1024 * 1024 * 1024
         deb_url = ('http://snapshot.debian.org/archive/debian/'
                    '20200108T145233Z/pool/main/u/u-boot/'
                    'u-boot-sunxi_2020.01%2Bdfsg-1_armhf.deb')
@@ -886,7 +891,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
         image_path_gz = self.fetch_asset(image_url, asset_hash=image_hash)
         image_path = os.path.join(self.workdir, 'armv7.img')
         archive.gzip_uncompress(image_path_gz, image_path)
-        image_pow2ceil_expand(image_path)
+        image_expand(image_path, NETBSD_SDCARD_MINSIZE)
         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
-- 
2.31.1

Re: [PATCH 5/9] tests/acceptance: Use image_expand() in test_arm_orangepi_uboot_netbsd9

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The NetBSD OrangePi image must be at least 2GiB, not less.
> Expand the SD card image to this size before using it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [PATCH 5/9] tests/acceptance: Use image_expand() in test_arm_orangepi_uboot_netbsd9

[Philippe Mathieu-Daudé]

Hi Niek,

On 6/23/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> The NetBSD OrangePi image must be at least 2GiB, not less.
> Expand the SD card image to this size before using it.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
> index 61069f0064f..b10f7257503 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -868,7 +868,12 @@ def test_arm_orangepi_uboot_netbsd9(self):
>          :avocado: tags=device:sd
>          :avocado: tags=os:netbsd
>          """
> -        # This test download a 304MB compressed image and expand it to 2GB
> +        # This test download a 304MB compressed image and expand it to 2GB,
> +        # which is the minimum card size required by the NetBSD installer:
> +        # https://wiki.netbsd.org/ports/evbarm/raspberry_pi/#index7h2
> +        # "A 2 GB card is the smallest workable size that the installation
> +        # image will fit on."

Do you agree with this comment and the one in the next patch?

> +        NETBSD_SDCARD_MINSIZE = 2 * 1024 * 1024 * 1024
>          deb_url = ('http://snapshot.debian.org/archive/debian/'
>                     '20200108T145233Z/pool/main/u/u-boot/'
>                     'u-boot-sunxi_2020.01%2Bdfsg-1_armhf.deb')
> @@ -886,7 +891,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
>          image_path_gz = self.fetch_asset(image_url, asset_hash=image_hash)
>          image_path = os.path.join(self.workdir, 'armv7.img')
>          archive.gzip_uncompress(image_path_gz, image_path)
> -        image_pow2ceil_expand(image_path)
> +        image_expand(image_path, NETBSD_SDCARD_MINSIZE)
>          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
> 

Re: [PATCH 5/9] tests/acceptance: Use image_expand() in test_arm_orangepi_uboot_netbsd9

[Niek Linnenbank]

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

Hi Philippe,

Op wo 4 aug. 2021 22:55 schreef Philippe Mathieu-Daudé <f4bug@amsat.org>:

> Hi Niek,
>
> On 6/23/21 8:00 PM, Philippe Mathieu-Daudé wrote:
> > The NetBSD OrangePi image must be at least 2GiB, not less.
> > Expand the SD card image to this size before using it.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  tests/acceptance/boot_linux_console.py | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> > index 61069f0064f..b10f7257503 100644
> > --- a/tests/acceptance/boot_linux_console.py
> > +++ b/tests/acceptance/boot_linux_console.py
> > @@ -868,7 +868,12 @@ def test_arm_orangepi_uboot_netbsd9(self):
> >          :avocado: tags=device:sd
> >          :avocado: tags=os:netbsd
> >          """
> > -        # This test download a 304MB compressed image and expand it to
> 2GB
> > +        # This test download a 304MB compressed image and expand it to
> 2GB,
> > +        # which is the minimum card size required by the NetBSD
> installer:
> > +        # https://wiki.netbsd.org/ports/evbarm/raspberry_pi/#index7h2
> > +        # "A 2 GB card is the smallest workable size that the
> installation
> > +        # image will fit on."
>
> Do you agree with this comment and the one in the next patch?
>

Yes, this change looks fine for me.

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>


> > +        NETBSD_SDCARD_MINSIZE = 2 * 1024 * 1024 * 1024
> >          deb_url = ('http://snapshot.debian.org/archive/debian/'
> >                     '20200108T145233Z/pool/main/u/u-boot/'
> >                     'u-boot-sunxi_2020.01%2Bdfsg-1_armhf.deb')
> > @@ -886,7 +891,7 @@ def test_arm_orangepi_uboot_netbsd9(self):
> >          image_path_gz = self.fetch_asset(image_url,
> asset_hash=image_hash)
> >          image_path = os.path.join(self.workdir, 'armv7.img')
> >          archive.gzip_uncompress(image_path_gz, image_path)
> > -        image_pow2ceil_expand(image_path)
> > +        image_expand(image_path, NETBSD_SDCARD_MINSIZE)
> >          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
> >
>
>

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

[RFC PATCH 6/9] tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08

[Philippe Mathieu-Daudé]

U-Boot expects the SD card size to be at least 2GiB, so
expand the SD card image to this size before using it.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
TODO: U-Boot reference?
---
 tests/acceptance/boot_linux_console.py | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index b10f7257503..c4c0f0b393d 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -820,11 +820,13 @@ def test_arm_orangepi_bionic_20_08(self):
         :avocado: tags=arch:arm
         :avocado: tags=machine:orangepi-pc
         :avocado: tags=device:sd
+        :avocado: tags=u-boot
         """
 
-        # This test download a 275 MiB compressed image and expand it
-        # to 1036 MiB, but the underlying filesystem is 1552 MiB...
-        # As we expand it to 2 GiB we are safe.
+        # This test download a 275 MiB compressed image and inflate it
+        # to 1036 MiB, but 1/ the underlying filesystem is 1552 MiB,
+        # 2/ U-Boot loads TFTP filenames from the last sector below
+        # 2 GiB, so we need to expand the image further more to 2 GiB.
 
         image_url = ('https://archive.armbian.com/orangepipc/archive/'
                      'Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz')
@@ -833,7 +835,7 @@ def test_arm_orangepi_bionic_20_08(self):
         image_path_xz = self.fetch_asset(image_url, asset_hash=image_hash,
                                          algorithm='sha256')
         image_path = archive.extract(image_path_xz, self.workdir)
-        image_pow2ceil_expand(image_path)
+        image_expand(image_path, 2 * 1024 * 1024 * 1024)
 
         self.vm.set_console()
         self.vm.add_args('-drive', 'file=' + image_path + ',if=sd,format=raw',
-- 
2.31.1

Re: [RFC PATCH 6/9] tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:09 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> U-Boot expects the SD card size to be at least 2GiB, so
> expand the SD card image to this size before using it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> TODO: U-Boot reference?
> ---
>  tests/acceptance/boot_linux_console.py | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

Re: [RFC PATCH 6/9] tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08

[Niek Linnenbank]

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

Hi Philippe,

Op wo 23 jun. 2021 20:00 schreef Philippe Mathieu-Daudé <f4bug@amsat.org>:

> U-Boot expects the SD card size to be at least 2GiB, so
> expand the SD card image to this size before using it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> TODO: U-Boot reference?
> ---
>  tests/acceptance/boot_linux_console.py | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tests/acceptance/boot_linux_console.py
> b/tests/acceptance/boot_linux_console.py
> index b10f7257503..c4c0f0b393d 100644
> --- a/tests/acceptance/boot_linux_console.py
> +++ b/tests/acceptance/boot_linux_console.py
> @@ -820,11 +820,13 @@ def test_arm_orangepi_bionic_20_08(self):
>          :avocado: tags=arch:arm
>          :avocado: tags=machine:orangepi-pc
>          :avocado: tags=device:sd
> +        :avocado: tags=u-boot
>          """
>
> -        # This test download a 275 MiB compressed image and expand it
> -        # to 1036 MiB, but the underlying filesystem is 1552 MiB...
> -        # As we expand it to 2 GiB we are safe.
> +        # This test download a 275 MiB compressed image and inflate it
>

Only a few typos here:
  download -> downloads
  inflate -> inflates

Otherwise, looks fine:

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

+        # to 1036 MiB, but 1/ the underlying filesystem is 1552 MiB,
> +        # 2/ U-Boot loads TFTP filenames from the last sector below
> +        # 2 GiB, so we need to expand the image further more to 2 GiB.
>
>          image_url = ('https://archive.armbian.com/orangepipc/archive/'
>
> 'Armbian_20.08.1_Orangepipc_bionic_current_5.8.5.img.xz')
> @@ -833,7 +835,7 @@ def test_arm_orangepi_bionic_20_08(self):
>          image_path_xz = self.fetch_asset(image_url, asset_hash=image_hash,
>                                           algorithm='sha256')
>          image_path = archive.extract(image_path_xz, self.workdir)
> -        image_pow2ceil_expand(image_path)
> +        image_expand(image_path, 2 * 1024 * 1024 * 1024)
>
>          self.vm.set_console()
>          self.vm.add_args('-drive', 'file=' + image_path +
> ',if=sd,format=raw',
> --
> 2.31.1
>
>

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

[RFC PATCH 7/9] tests/acceptance: Do not expand SD card image in test_arm_orangepi_sd

[Philippe Mathieu-Daudé]

XXX it seems to work...

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

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index c4c0f0b393d..48c0ba09117 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -783,7 +783,6 @@ 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 +
-- 
2.31.1

[PATCH 8/9] tests/acceptance: Remove now unused pow2ceil()

[Philippe Mathieu-Daudé]

We don't use pow2ceil() anymore, remove it.

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

diff --git a/tests/acceptance/boot_linux_console.py b/tests/acceptance/boot_linux_console.py
index 48c0ba09117..77bc80c505d 100644
--- a/tests/acceptance/boot_linux_console.py
+++ b/tests/acceptance/boot_linux_console.py
@@ -29,12 +29,6 @@
 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
 """
@@ -43,12 +37,6 @@ def image_expand(path, size):
         with open(path, 'ab+') as fd:
             fd.truncate(size)
 
-"""
-Expand file size to next power of 2
-"""
-def image_pow2ceil_expand(path):
-    image_expand(path, pow2ceil(os.path.getsize(path)))
-
 class LinuxKernelTest(Test):
     KERNEL_COMMON_COMMAND_LINE = 'printk.time=0 '
 
-- 
2.31.1

Re: [PATCH 8/9] tests/acceptance: Remove now unused pow2ceil()

[Willian Rampazzo]

On Wed, Jun 23, 2021 at 3:10 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We don't use pow2ceil() anymore, remove it.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  tests/acceptance/boot_linux_console.py | 12 ------------
>  1 file changed, 12 deletions(-)
>

Reviewed-by: Willian Rampazzo <willianr@redhat.com>

[RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again

[Philippe Mathieu-Daudé]

In commit a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card
sizes") we tried to protect us from CVE-2020-13253 by only allowing
card with power-of-2 sizes. However doing so we disrupted valid user
cases. As a compromise, allow any card size, but warn only power of 2
sizes are supported, still suggesting the user how to increase a
card with 'qemu-img resize'.

Cc: Tom Yan <tom.ty89@gmail.com>
Cc: Warner Losh <imp@bsdimp.com>
Buglink: https://bugs.launchpad.net/qemu/+bug/1910586
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9c8dd11bad1..cab4aab1475 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2131,23 +2131,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
         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;
+            g_autofree char *blk_size_s = size_to_str(blk_size);
+            g_autofree char *blk_size_aligned_s = size_to_str(blk_size_aligned);
 
-            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;
+            warn_report("SD card size is not a power of 2 (%s). It might work"
+                        " but is not supported by QEMU. If possible, resize"
+                        " your disk image to %s with:",
+                        blk_size_s, blk_size_aligned_s);
+            warn_report(" 'qemu-img resize <imagefile> <new-size>'");
+            warn_report("(note that this will lose data if you make the"
+                        " image smaller than it currently is).");
         }
 
         ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
-- 
2.31.1

Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again

[Daniel P. Berrangé]

On Wed, Jun 23, 2021 at 08:00:21PM +0200, Philippe Mathieu-Daudé wrote:
> In commit a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card
> sizes") we tried to protect us from CVE-2020-13253 by only allowing
> card with power-of-2 sizes. However doing so we disrupted valid user
> cases. As a compromise, allow any card size, but warn only power of 2
> sizes are supported, still suggesting the user how to increase a
> card with 'qemu-img resize'.
> 
> Cc: Tom Yan <tom.ty89@gmail.com>
> Cc: Warner Losh <imp@bsdimp.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1910586
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9c8dd11bad1..cab4aab1475 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2131,23 +2131,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          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;
> +            g_autofree char *blk_size_s = size_to_str(blk_size);
> +            g_autofree char *blk_size_aligned_s = size_to_str(blk_size_aligned);
>  
> -            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;
> +            warn_report("SD card size is not a power of 2 (%s). It might work"
> +                        " but is not supported by QEMU. If possible, resize"
> +                        " your disk image to %s with:",
> +                        blk_size_s, blk_size_aligned_s);
> +            warn_report(" 'qemu-img resize <imagefile> <new-size>'");
> +            warn_report("(note that this will lose data if you make the"
> +                        " image smaller than it currently is).");

In what scenarios will non-power of 2 not work and what is the effect ?
Is it a QEMU bug or not ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again

[Tom Yan]

Hi,

On Thu, 24 Jun 2021 at 02:01, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> In commit a9bcedd15a5 ("hw/sd/sdcard: Do not allow invalid SD card
> sizes") we tried to protect us from CVE-2020-13253 by only allowing
> card with power-of-2 sizes. However doing so we disrupted valid user
> cases. As a compromise, allow any card size, but warn only power of 2
> sizes are supported, still suggesting the user how to increase a
> card with 'qemu-img resize'.
>
> Cc: Tom Yan <tom.ty89@gmail.com>
> Cc: Warner Losh <imp@bsdimp.com>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1910586
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 9c8dd11bad1..cab4aab1475 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2131,23 +2131,16 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          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;
> +            g_autofree char *blk_size_s = size_to_str(blk_size);
> +            g_autofree char *blk_size_aligned_s = size_to_str(blk_size_aligned);
>
> -            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;
> +            warn_report("SD card size is not a power of 2 (%s). It might work"
> +                        " but is not supported by QEMU. If possible, resize"
> +                        " your disk image to %s with:",
> +                        blk_size_s, blk_size_aligned_s);
> +            warn_report(" 'qemu-img resize <imagefile> <new-size>'");
> +            warn_report("(note that this will lose data if you make the"
> +                        " image smaller than it currently is).");

Not trying to be picky, but I don't think this is much better. IMHO
it's quite irresponsible to give a warning like that, leaving users in
a state like "Should I use it or not then?", without giving a concrete
reference to what exactly might/would lead to the warned problem.

I really think we should get (/ have gotten) things clear first. What
exactly is the bug we have been talking about here? I mean like, where
does it occur and what's the nature of it.

1. Is it specific to a certain type / model of backend / physical
storage device that will be made use of by qemu for the emulated
storage? (I presume not since you mention about image, unless you
irrationally related/bound the emulated storage type and the physical
storage type together.)

2. Does it have anything to do with a certain flaw in qemu itself?
Like the code that does read/write operation is flawed that it cannot
be handled by a certain *proper* backend device?

3. Or is it actually a bug in a certain driver / firmware blob that
will be used by an *emulated* device in the guest? In that case, can
we emulate another model so that it won't be using the problematic
driver / firmware?

Also, could you provide any information / reference to what the bug
really is? Like a bug report (for the problem itself, not some vague
claim that qemu should workaround the problem)?

>          }
>
>          ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
> --
> 2.31.1
>

Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again

[Peter Maydell]

On Thu, 24 Jun 2021 at 11:27, Tom Yan <tom.ty89@gmail.com> wrote:
> I really think we should get (/ have gotten) things clear first. What
> exactly is the bug we have been talking about here? I mean like, where
> does it occur and what's the nature of it.
>
> 1. Is it specific to a certain type / model of backend / physical
> storage device that will be made use of by qemu for the emulated
> storage? (I presume not since you mention about image, unless you
> irrationally related/bound the emulated storage type and the physical
> storage type together.)
>
> 2. Does it have anything to do with a certain flaw in qemu itself?
> Like the code that does read/write operation is flawed that it cannot
> be handled by a certain *proper* backend device?
>
> 3. Or is it actually a bug in a certain driver / firmware blob that
> will be used by an *emulated* device in the guest? In that case, can
> we emulate another model so that it won't be using the problematic
> driver / firmware?

Definitely agreed -- before we start changing QEMU code we need
to identify clearly (a) what the real hardware does and (b) what
the situation was we were originally trying to fix.

thanks
-- PMM

Re: [RFC PATCH 9/9] hw/sd: Allow card size not power of 2 again

[Warner Losh]

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



> On Jun 24, 2021, at 4:56 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Thu, 24 Jun 2021 at 11:27, Tom Yan <tom.ty89@gmail.com> wrote:
>> I really think we should get (/ have gotten) things clear first. What
>> exactly is the bug we have been talking about here? I mean like, where
>> does it occur and what's the nature of it.
>> 
>> 1. Is it specific to a certain type / model of backend / physical
>> storage device that will be made use of by qemu for the emulated
>> storage? (I presume not since you mention about image, unless you
>> irrationally related/bound the emulated storage type and the physical
>> storage type together.)
>> 
>> 2. Does it have anything to do with a certain flaw in qemu itself?
>> Like the code that does read/write operation is flawed that it cannot
>> be handled by a certain *proper* backend device?
>> 
>> 3. Or is it actually a bug in a certain driver / firmware blob that
>> will be used by an *emulated* device in the guest? In that case, can
>> we emulate another model so that it won't be using the problematic
>> driver / firmware?
> 
> Definitely agreed -- before we start changing QEMU code we need
> to identify clearly (a) what the real hardware does and (b) what
> the situation was we were originally trying to fix.

Real SD and MMC cards do not have power-of-two constraints in the
number of blocks. None of the SD/MMC bridges I’ve ever dealt with
have a power-of-two constraint on the size of the card. The only
constraint on the size related to the card is the block size.

If non-power-of-2 sized cards fail, that’s a cat-1 sev-1 bug that needs
to be fixed, rather than a warning be generated.

Warner

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 874 bytes --]

Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again

[Alexander Bulekov]

On 210623 2000, Philippe Mathieu-Daudé wrote:
> Hi Ubi-Wan Kenubi and Tom,
> 
> In commit a9bcedd (SD card size has to be power of 2) we decided
> to restrict SD card size to avoid security problems (CVE-2020-13253)
> but this became not practical to some users.
> 
> This RFC series tries to remove the limitation, keeping our
> functional tests working. It is unfinished work because I had to
> attend other topics, but sending it early as RFC to get feedback.
> I'll keep working when I get more time, except if one if you can
> help me.
> 
> Alexander, could you generate a qtest reproducer with the fuzzer
> corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054

I think that bug was already fixed - the reproducer no logner causes a
timeout on 6.0. Did I misunderstand something?

I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3
config. The only problem it found is this assert() (that exists without the
patch anyways):
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 
Let me know if this is something you think I should report on gitlab..

I'll leave the fuzzer running for another 24h or so, but otherwise I'm
happy to leave a Tested-by, once there is a V1 series 
-Alex



> Thanks,
> 
> Phil.
> 
> Philippe Mathieu-Daudé (9):
>   hw/sd: When card is in wrong state, log which state it is
>   hw/sd: Extract address_in_range() helper, log invalid accesses
>   tests/acceptance: Tag NetBSD tests as 'os:netbsd'
>   tests/acceptance: Extract image_expand() helper
>   tests/acceptance: Use image_expand() in
>     test_arm_orangepi_uboot_netbsd9
>   tests/acceptance: Use image_expand() in test_arm_orangepi_bionic_20_08
>   tests/acceptance: Do not expand SD card image in test_arm_orangepi_sd
>   tests/acceptance: Remove now unused pow2ceil()
>   hw/sd: Allow card size not power of 2 again
> 
>  hw/sd/sd.c                             | 60 +++++++++++++-------------
>  tests/acceptance/boot_linux_console.py | 39 ++++++++---------
>  tests/acceptance/ppc_prep_40p.py       |  2 +
>  3 files changed, 52 insertions(+), 49 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again

[Philippe Mathieu-Daudé]

On 6/24/21 4:50 AM, Alexander Bulekov wrote:
> On 210623 2000, Philippe Mathieu-Daudé wrote:
>> Hi Ubi-Wan Kenubi and Tom,
>>
>> In commit a9bcedd (SD card size has to be power of 2) we decided
>> to restrict SD card size to avoid security problems (CVE-2020-13253)
>> but this became not practical to some users.
>>
>> This RFC series tries to remove the limitation, keeping our
>> functional tests working. It is unfinished work because I had to
>> attend other topics, but sending it early as RFC to get feedback.
>> I'll keep working when I get more time, except if one if you can
>> help me.
>>
>> Alexander, could you generate a qtest reproducer with the fuzzer
>> corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054
> 
> I think that bug was already fixed - the reproducer no logner causes a
> timeout on 6.0. Did I misunderstand something?

That bug was fixed but now I'm changing the code and would like to feel
sure I'm not re-introducing the problem, so having the reproducer in the
tree would help.

> I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3
> config. The only problem it found is this assert() (that exists without the
> patch anyways):
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 

Sigh.

> Let me know if this is something you think I should report on gitlab..

Yes please :(

> I'll leave the fuzzer running for another 24h or so, but otherwise I'm
> happy to leave a Tested-by, once there is a V1 series 
> -Alex

Thanks!

Phil.

Re: [RFC PATCH 0/9] hw/sd: Allow card size not power of 2 again

[Alexander Bulekov]

On 210624 1012, Philippe Mathieu-Daudé wrote:
> On 6/24/21 4:50 AM, Alexander Bulekov wrote:
> > On 210623 2000, Philippe Mathieu-Daudé wrote:
> >> Hi Ubi-Wan Kenubi and Tom,
> >>
> >> In commit a9bcedd (SD card size has to be power of 2) we decided
> >> to restrict SD card size to avoid security problems (CVE-2020-13253)
> >> but this became not practical to some users.
> >>
> >> This RFC series tries to remove the limitation, keeping our
> >> functional tests working. It is unfinished work because I had to
> >> attend other topics, but sending it early as RFC to get feedback.
> >> I'll keep working when I get more time, except if one if you can
> >> help me.
> >>
> >> Alexander, could you generate a qtest reproducer with the fuzzer
> >> corpus? See: https://bugs.launchpad.net/qemu/+bug/1878054
> > 
> > I think that bug was already fixed - the reproducer no logner causes a
> > timeout on 6.0. Did I misunderstand something?
> 
> That bug was fixed but now I'm changing the code and would like to feel
> sure I'm not re-introducing the problem, so having the reproducer in the
> tree would help.

Ok - I'll try to come up with one. Minimizing reproducers for timeouts
is trickier than crashes :-)

> 
> > I applied this series and ran the OSS-Fuzz corpus for the sdhci-v3
> > config. The only problem it found is this assert() (that exists without the
> > patch anyways):
> > https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=29225 
> 
> Sigh.
> 
> > Let me know if this is something you think I should report on gitlab..
> 
> Yes please :(

Ok here it is: https://gitlab.com/qemu-project/qemu/-/issues/450

The fuzzer I left running for 24h also found another bug (looks like DMA
reentrancy), which I thought was introduced by this patchset, since
OSS-Fuzz had not reported it in months of fuzzing. However, as a
sanity-check I tried it against qemu.git and it crashed - strange...
Anyways here it is: https://gitlab.com/qemu-project/qemu/-/issues/451

-Alex

> 
> > I'll leave the fuzzer running for another 24h or so, but otherwise I'm
> > happy to leave a Tested-by, once there is a V1 series 
> > -Alex
> 
> Thanks!
> 
> Phil.