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

[Paolo Bonzini]

On 18/01/21 07:32, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While processing ATAPI cmd_read/cmd_read_cd commands,
> Logical Block Address (LBA) maybe invalid OR closer to the last block,
> leading to an OOB access issues. Add range check to avoid it.
>
> Fixes: CVE-2020-29443
> Reported-by: Wenxiang Qian <leonwxqian@gmail.com>
> Fix-suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>

Thank you!  This looks great.

With the small spacing fix suggested by checkpatch,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

You may add a small patch on top to clamp s->nb_sectors at 
(uint64_t)INT_MAX << 2, just to be super safe.

Paolo


> ---
>   hw/ide/atapi.c | 30 ++++++++++++++++++++++++------
>   1 file changed, 24 insertions(+), 6 deletions(-)
>
> Update v2: range check lba value early in cmd_read[_cd] routines
>    -> https://lists.gnu.org/archive/html/qemu-devel/2020-12/msg00151.html
>
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index e79157863f..35b8494dc8 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -322,6 +322,8 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int 
> max_size)
>   static void ide_atapi_cmd_read_pio(IDEState *s, int lba, int nb_sectors,
>                                      int sector_size)
>   {
> +    assert (0 <= lba && lba < (s->nb_sectors >> 2));
> +
>       s->lba = lba;
>       s->packet_transfer_size = nb_sectors * sector_size;
>       s->elementary_transfer_size = 0;
> @@ -420,6 +422,8 @@ eot:
>   static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
>                                      int sector_size)
>   {
> +    assert (0 <= lba && lba < (s->nb_sectors >> 2));
> +
>       s->lba = lba;
>       s->packet_transfer_size = nb_sectors * sector_size;
>       s->io_buffer_size = 0;
> @@ -973,35 +977,49 @@ static void cmd_prevent_allow_medium_removal(IDEState *s, 
> uint8_t* buf)
>
>   static void cmd_read(IDEState *s, uint8_t* buf)
>   {
> -    int nb_sectors, lba;
> +    unsigned int nb_sectors, lba;
> +
> +    /* Total logical sectors of ATAPI_SECTOR_SIZE(=2048) bytes */
> +    uint64_t total_sectors = s->nb_sectors >> 2;
>
>       if (buf[0] == GPCMD_READ_10) {
>           nb_sectors = lduw_be_p(buf + 7);
>       } else {
>           nb_sectors = ldl_be_p(buf + 6);
>       }
> -
> -    lba = ldl_be_p(buf + 2);
>       if (nb_sectors == 0) {
>           ide_atapi_cmd_ok(s);
>           return;
>       }
>
> +    lba = ldl_be_p(buf + 2);
> +    if (lba >= total_sectors || lba + nb_sectors - 1 >= total_sectors) {
> +        ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
> +        return;
> +    }
> +
>       ide_atapi_cmd_read(s, lba, nb_sectors, 2048);
>   }
>
>   static void cmd_read_cd(IDEState *s, uint8_t* buf)
>   {
> -    int nb_sectors, lba, transfer_request;
> +    unsigned int nb_sectors, lba, transfer_request;
> +
> +    /* Total logical sectors of ATAPI_SECTOR_SIZE(=2048) bytes */
> +    uint64_t total_sectors = s->nb_sectors >> 2;
>
>       nb_sectors = (buf[6] << 16) | (buf[7] << 8) | buf[8];
> -    lba = ldl_be_p(buf + 2);
> -
>       if (nb_sectors == 0) {
>           ide_atapi_cmd_ok(s);
>           return;
>       }
>
> +    lba = ldl_be_p(buf + 2);
> +    if (lba >= total_sectors || lba + nb_sectors - 1 >= total_sectors) {
> +        ide_atapi_cmd_error(s, ILLEGAL_REQUEST, ASC_LOGICAL_BLOCK_OOR);
> +        return;
> +    }
> +
>       transfer_request = buf[9] & 0xf8;
>       if (transfer_request == 0x00) {
>           /* nothing */
> --
> 2.29.2