Re: [PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)

[Paolo Bonzini]

On 18/01/21 13:29, P J P wrote:
> +-- On Mon, 18 Jan 2021, Paolo Bonzini wrote --+
> | s->nb_sectors is in units of 512B, so the limit would be 4TB.  The purpose
> | is to limit the lba and nb_sectors arguments (which are in 2048B units) of
> | ide_atapi_cmd_read_{dma,pio} to INT_MAX.
>
> * If it's for IDE_CD type, does the patch below look okay?

Not an &, but rather a MIN.

There is also ide_resize_cb, so perhaps a new function 
ide_set_nb_sectors in hw/ide/core.c would be better.

> ===
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index b49e4cfbc6..034c84b350 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -1169,7 +1169,7 @@ static void ide_cd_change_cb(void *opaque, bool load,
> Error **errp)
>   
>       s->tray_open = !load;
>       blk_get_geometry(s->blk, &nb_sectors);
> -    s->nb_sectors = nb_sectors;
> +    s->nb_sectors = nb_sectors & (uint64_t)INT_MAX << 2;
>   
>       /*
>        * First indicate to the guest that a CD has been removed.  That's
> @@ -2530,6 +2530,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk,
> IDEDriveKind kind,
>       s->smart_errors = 0;
>       s->smart_selftest_count = 0;
>       if (kind == IDE_CD) {
> +        s->nb_sectors &= (uint64_t)INT_MAX << 2;
>           blk_set_dev_ops(blk, &ide_cd_block_ops, s);
>           blk_set_guest_block_size(blk, 2048);
> ===
>
> * Isn't 4TB limit more for IDE_CD type? Maybe UINT32_MAX?

Yes it's a lot more than the practical limit but it doesn't hurt either 
to have INT_MAX << 2.  The point is to have a limit that makes sense as 
far as the atapi.c code is concerned, i.e. to ensure the size in 
2048-byte sectors fits an "int".

Paolo