Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes
Peter Maydell writes: > On Mon, 13 Jul 2020 at 19:32, Philippe Mathieu-Daudé wrote: >> >> QEMU allows to create SD card with unrealistic sizes. This could >> work, but some guests (at least Linux) consider sizes that are not >> a power of 2 as a firmware bug and fix the card size to the next >> power of 2. >> > >> +error_setg(errp, "Invalid SD card size: %s", blk_size_str); >> +g_free(blk_size_str); >> + >> +blk_size_str = size_to_str(blk_size_aligned); >> +error_append_hint(errp, >> + "SD card size has to be a power of 2, e.g. >> %s.\n" >> + "You can resize disk images with " >> + "'qemu-img resize '\n" >> + "(note that this will lose data if you make >> the " >> + "image smaller than it currently is).\n", >> + blk_size_str); >> +g_free(blk_size_str); > > Some places that create multi-line hints with error_append_hint() > do it by calling it once per line, eg in target/arm/cpu64.c: > error_setg(errp, "cannot disable sve128"); > error_append_hint(errp, "Disabling sve128 results in all " > "vector lengths being disabled.\n"); > error_append_hint(errp, "With SVE enabled, at least one " > "vector length must be enabled.\n"); > > Some places don't, eg in block/vhdx-log.c: > error_append_hint(errp, "To replay the log, run:\n" > "qemu-img check -r all '%s'\n", > bs->filename); Both fine. > Most places terminate with a '\n', but some places don't, eg > crypto/block-luks.c: >error_append_hint(errp, "Failed to write to keyslot %i", keyslot); >return -1; This is a bug. > The documentation says > * May be called multiple times. The resulting hint should end with a > * newline. > > which isn't very clear -- you can call it multiple times, but > must you, if it's multiline? If that was required, my doc comment would demand it. > I assume that "should end with a newline" means "must end > with a newline", and places like block-luks.c are bugs. If you construct a hint with multiple calls, individual calls need not terminate with a newline, but the resulting hint still should. I readily admit that my doc comments sometimes require a "lawyerly" reading :) > Markus, do you know what the intended API here is? > > It looks like the implementation just tacks the hint > string onto the end of any existing hint string, in > which case multiple-line strings are fine and the same > behaviour as calling the function multiple times. That's the intended behavior. > (I had assumed we might be accumulating an array of strings, > or requiring multiline strings to be multiple calls so we > could have the argument not need to be \n-terminated, > to match error_setg(), but both those assumptions > are obviously wrong.) Would also be workable, if slightly less flexible. Yes, error_setg() and error_append_hint() differ on newlines. error_setg() constructs an error message. These get reported in a rigid format that does not afford line breaks in the middle. error_append_hint() builds up a hint. Hints are free-form text. Embedded line breaks are fine. In both cases, I decided not to treat newline at the end any different from newlines anywhere else. Thus, no newlines at all with error_setg(), and newlines after every line including the last one with error_append_hint(). > Anyway, I guess this multiline-message usage is something > we do already and it will DTRT, so Correct. Hope this helps! > Reviewed-by: Peter Maydell > > > thanks > -- PMM
Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes
On Mon, 13 Jul 2020 at 19:32, Philippe Mathieu-Daudé wrote: > > QEMU allows to create SD card with unrealistic sizes. This could > work, but some guests (at least Linux) consider sizes that are not > a power of 2 as a firmware bug and fix the card size to the next > power of 2. > > +error_setg(errp, "Invalid SD card size: %s", blk_size_str); > +g_free(blk_size_str); > + > +blk_size_str = size_to_str(blk_size_aligned); > +error_append_hint(errp, > + "SD card size has to be a power of 2, e.g. > %s.\n" > + "You can resize disk images with " > + "'qemu-img resize '\n" > + "(note that this will lose data if you make > the " > + "image smaller than it currently is).\n", > + blk_size_str); > +g_free(blk_size_str); Some places that create multi-line hints with error_append_hint() do it by calling it once per line, eg in target/arm/cpu64.c: error_setg(errp, "cannot disable sve128"); error_append_hint(errp, "Disabling sve128 results in all " "vector lengths being disabled.\n"); error_append_hint(errp, "With SVE enabled, at least one " "vector length must be enabled.\n"); Some places don't, eg in block/vhdx-log.c: error_append_hint(errp, "To replay the log, run:\n" "qemu-img check -r all '%s'\n", bs->filename); Most places terminate with a '\n', but some places don't, eg crypto/block-luks.c: error_append_hint(errp, "Failed to write to keyslot %i", keyslot); return -1; The documentation says * May be called multiple times. The resulting hint should end with a * newline. which isn't very clear -- you can call it multiple times, but must you, if it's multiline? I assume that "should end with a newline" means "must end with a newline", and places like block-luks.c are bugs. Markus, do you know what the intended API here is? It looks like the implementation just tacks the hint string onto the end of any existing hint string, in which case multiple-line strings are fine and the same behaviour as calling the function multiple times. (I had assumed we might be accumulating an array of strings, or requiring multiline strings to be multiple calls so we could have the argument not need to be \n-terminated, to match error_setg(), but both those assumptions are obviously wrong.) Anyway, I guess this multiline-message usage is something we do already and it will DTRT, so Reviewed-by: Peter Maydell thanks -- PMM
Re: [PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes
On Mon, Jul 13, 2020 at 11:33 AM Philippe Mathieu-Daudé wrote:
>
> QEMU allows to create SD card with unrealistic sizes. This could
> work, but some guests (at least Linux) consider sizes that are not
> a power of 2 as a firmware bug and fix the card size to the next
> power of 2.
>
> While the possibility to use small SD card images has been seen as
> a feature, it became a bug with CVE-2020-13253, where the guest is
> able to do OOB read/write accesses past the image size end.
>
> In a pair of commits we will fix CVE-2020-13253 as:
>
> Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> occurred and no data transfer is performed.
>
> Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
> occurred and no data transfer is performed.
>
> WP_VIOLATION errors are not modified: the error bit is set, we
> stay in receive-data state, wait for a stop command. All further
> data transfer is ignored. See the check on sd->card_status at the
> beginning of sd_read_data() and sd_write_data().
>
> While this is the correct behavior, in case QEMU create smaller SD
> cards, guests still try to access past the image size end, and QEMU
> considers this is an invalid address, thus "all further data transfer
> is ignored". This is wrong and make the guest looping until
> eventually timeouts.
>
> Fix by not allowing invalid SD card sizes (suggesting the expected
> size as a hint):
>
> $ qemu-system-arm -M orangepi-pc -drive file=rootfs.ext2,if=sd,format=raw
> qemu-system-arm: Invalid SD card size: 60 MiB
> SD card size has to be a power of 2, e.g. 64 MiB.
> You can resize disk images with 'qemu-img resize '
> (note that this will lose data if you make the image smaller than it
> currently is).
>
> Signed-off-by: Philippe Mathieu-Daudé
Reviewed-by: Alistair Francis
Alistair
> ---
> Since v1:
> Addressed Alistair & Peter comments (error_append_hint message)
> ---
> hw/sd/sd.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index edd60a09c0..5ab945dade 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -32,6 +32,7 @@
>
> #include "qemu/osdep.h"
> #include "qemu/units.h"
> +#include "qemu/cutils.h"
> #include "hw/irq.h"
> #include "hw/registerfields.h"
> #include "sysemu/block-backend.h"
> @@ -2106,11 +2107,35 @@ static void sd_realize(DeviceState *dev, Error **errp)
> }
>
> if (sd->blk) {
> +int64_t blk_size;
> +
> if (blk_is_read_only(sd->blk)) {
> error_setg(errp, "Cannot use read-only drive as SD card");
> return;
> }
>
> +blk_size = blk_getlength(sd->blk);
> +if (blk_size > 0 && !is_power_of_2(blk_size)) {
> +int64_t blk_size_aligned = pow2ceil(blk_size);
> +char *blk_size_str;
> +
> +blk_size_str = size_to_str(blk_size);
> +error_setg(errp, "Invalid SD card size: %s", blk_size_str);
> +g_free(blk_size_str);
> +
> +blk_size_str = size_to_str(blk_size_aligned);
> +error_append_hint(errp,
> + "SD card size has to be a power of 2, e.g.
> %s.\n"
> + "You can resize disk images with "
> + "'qemu-img resize '\n"
> + "(note that this will lose data if you make
> the "
> + "image smaller than it currently is).\n",
> + blk_size_str);
> +g_free(blk_size_str);
> +
> +return;
> +}
> +
> ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
> BLK_PERM_ALL, errp);
> if (ret < 0) {
> --
> 2.21.3
>
>
[PATCH v2 7/9] hw/sd/sdcard: Do not allow invalid SD card sizes
QEMU allows to create SD card with unrealistic sizes. This could
work, but some guests (at least Linux) consider sizes that are not
a power of 2 as a firmware bug and fix the card size to the next
power of 2.
While the possibility to use small SD card images has been seen as
a feature, it became a bug with CVE-2020-13253, where the guest is
able to do OOB read/write accesses past the image size end.
In a pair of commits we will fix CVE-2020-13253 as:
Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
occurred and no data transfer is performed.
Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
occurred and no data transfer is performed.
WP_VIOLATION errors are not modified: the error bit is set, we
stay in receive-data state, wait for a stop command. All further
data transfer is ignored. See the check on sd->card_status at the
beginning of sd_read_data() and sd_write_data().
While this is the correct behavior, in case QEMU create smaller SD
cards, guests still try to access past the image size end, and QEMU
considers this is an invalid address, thus "all further data transfer
is ignored". This is wrong and make the guest looping until
eventually timeouts.
Fix by not allowing invalid SD card sizes (suggesting the expected
size as a hint):
$ qemu-system-arm -M orangepi-pc -drive file=rootfs.ext2,if=sd,format=raw
qemu-system-arm: Invalid SD card size: 60 MiB
SD card size has to be a power of 2, e.g. 64 MiB.
You can resize disk images with 'qemu-img resize '
(note that this will lose data if you make the image smaller than it
currently is).
Signed-off-by: Philippe Mathieu-Daudé
---
Since v1:
Addressed Alistair & Peter comments (error_append_hint message)
---
hw/sd/sd.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index edd60a09c0..5ab945dade 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -32,6 +32,7 @@
#include "qemu/osdep.h"
#include "qemu/units.h"
+#include "qemu/cutils.h"
#include "hw/irq.h"
#include "hw/registerfields.h"
#include "sysemu/block-backend.h"
@@ -2106,11 +2107,35 @@ static void sd_realize(DeviceState *dev, Error **errp)
}
if (sd->blk) {
+int64_t blk_size;
+
if (blk_is_read_only(sd->blk)) {
error_setg(errp, "Cannot use read-only drive as SD card");
return;
}
+blk_size = blk_getlength(sd->blk);
+if (blk_size > 0 && !is_power_of_2(blk_size)) {
+int64_t blk_size_aligned = pow2ceil(blk_size);
+char *blk_size_str;
+
+blk_size_str = size_to_str(blk_size);
+error_setg(errp, "Invalid SD card size: %s", blk_size_str);
+g_free(blk_size_str);
+
+blk_size_str = size_to_str(blk_size_aligned);
+error_append_hint(errp,
+ "SD card size has to be a power of 2, e.g. %s.\n"
+ "You can resize disk images with "
+ "'qemu-img resize '\n"
+ "(note that this will lose data if you make the "
+ "image smaller than it currently is).\n",
+ blk_size_str);
+g_free(blk_size_str);
+
+return;
+}
+
ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
BLK_PERM_ALL, errp);
if (ret < 0) {
--
2.21.3
