Re: [PATCH-for-8.1] hw/sd/sdcard: Allow users to bypass the power-of-2 size check

[Peter Maydell]

On Mon, 17 Jul 2023 at 16:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Since we committed a9bcedd15a ("hw/sd/sdcard: Do not allow invalid
> SD card sizes") to preclude some guests to access beyond the size
> of the card (leading to security issues such CVE-2020-13253), various
> users complained this prevent them to run guests potencially well
> behaving with non-power-of-2 card sizes. In order to allow them to
> experiment with such guests, add a property to disable the pow2
> check.
>
> Resolves: https://bugs.launchpad.net/qemu/+bug/1910586
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/297
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1754
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

I agree with Daniel that this should be an x- property.
We still need to figure out what's actually going on here.

> @@ -2151,7 +2155,13 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          }
>
>          blk_size = blk_getlength(sd->blk);
> -        if (blk_size > 0 && !is_power_of_2(blk_size)) {
> +        if (sd->bypass_pow2_size_check) {
> +            warn_report_once("Unsupported property '%s' enabled: some guests"
> +                             " might trigger data corruption and/or crash"
> +                             " (thus this process is vulnerable to"
> +                             " CVE-2020-13253).",
> +                             PROP_NAME_BYPASS_POW2_SIZE_CHECK);

If the guest really can trigger an overrun of a QEMU buffer,
then we shouldn't be letting users opt in to this behaviour.
("Buggy guest might do something odd that causes it to get confused
or crash or just not get data out of the SD card" is fine.)
Again, we need to figure out what's actually happening here...

> +        } else if (blk_size > 0 && !is_power_of_2(blk_size)) {
>              int64_t blk_size_aligned = pow2ceil(blk_size);
>              char *blk_size_str;
>
> @@ -2161,11 +2171,15 @@ static void sd_realize(DeviceState *dev, Error **errp)
>
>              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"
> +                              "SD card size should 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",
> +                              " image smaller than it currently is).\n"
> +                              "Note: you can disable this check by setting"
> +                              " the '" PROP_NAME_BYPASS_POW2_SIZE_CHECK "'"
> +                              " global property but this is DANGEROUS"
> +                              " and unsupported.\n",

This is overly vague. In particular, we shouldn't
save the specifics of what the user is opting into
for when the user has actually enabled the x- option...

thanks
-- PMM