diff mbox series

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

Message ID 20210118063229.442350-1-ppandit@redhat.com
State New
Headers show
Series [v2] ide: atapi: check logical block address and read size (CVE-2020-29443) | expand

Commit Message

Prasad Pandit Jan. 18, 2021, 6:32 a.m. UTC 
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>
---
 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

--
2.29.2

Comments

no-reply@patchew.org Jan. 18, 2021, 6:39 a.m. UTC | #1 
Patchew URL: https://patchew.org/QEMU/20210118063229.442350-1-ppandit@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210118063229.442350-1-ppandit@redhat.com
Subject: [PATCH v2] ide: atapi: check logical block address and read size (CVE-2020-29443)

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210118063229.442350-1-ppandit@redhat.com -> patchew/20210118063229.442350-1-ppandit@redhat.com
Switched to a new branch 'test'
8f56049 ide: atapi: check logical block address and read size (CVE-2020-29443)

=== OUTPUT BEGIN ===
ERROR: space prohibited between function name and open parenthesis '('
#25: FILE: hw/ide/atapi.c:325:
+    assert (0 <= lba && lba < (s->nb_sectors >> 2));

ERROR: space prohibited between function name and open parenthesis '('
#34: FILE: hw/ide/atapi.c:425:
+    assert (0 <= lba && lba < (s->nb_sectors >> 2));

total: 2 errors, 0 warnings, 71 lines checked

Commit 8f560492c204 (ide: atapi: check logical block address and read size (CVE-2020-29443)) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210118063229.442350-1-ppandit@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Paolo Bonzini Jan. 18, 2021, 8:47 a.m. UTC | #2 
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
>
Philippe Mathieu-Daudé Jan. 18, 2021, 9:49 a.m. UTC | #3 
On 1/18/21 7:32 AM, 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>

"Suggested-by"

> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/ide/atapi.c | 30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
Prasad Pandit Jan. 18, 2021, 11:44 a.m. UTC | #4 
+-- On Mon, 18 Jan 2021, Paolo Bonzini wrote --+
| Thank you!  This looks great.
| With the small spacing fix suggested by checkpatch,
| Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

  Thank you. Will send patch v3 with space typo fix.

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

To confirm:

  * (uint64_t)INT_MAX << 2 is => 8589934588 ~= 8.5G sectors ?
    Media size would be:
      8.5G * 512B(sector) => ~4TB
      8.5G * 4096B(sector) => ~32TB

  * We are limiting IDE media size to ~4TB/~32TB ?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Paolo Bonzini Jan. 18, 2021, 11:57 a.m. UTC | #5 
On 18/01/21 12:44, P J P wrote:
> To confirm:
> 
>    * (uint64_t)INT_MAX << 2 is => 8589934588 ~= 8.5G sectors ?
>      Media size would be:
>        8.5G * 512B(sector) => ~4TB
>        8.5G * 4096B(sector) => ~32TB
> 
>    * We are limiting IDE media size to ~4TB/~32TB ?

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.

Paolo
Prasad Pandit Jan. 18, 2021, 12:29 p.m. UTC | #6 
+-- 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?
===
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?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
Paolo Bonzini Jan. 18, 2021, 12:46 p.m. UTC | #7 
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
Prasad Pandit Jan. 18, 2021, 1:06 p.m. UTC | #8 
+-- On Mon, 18 Jan 2021, Paolo Bonzini wrote --+
| On 18/01/21 13:29, P J P wrote:
| > +    s->nb_sectors = nb_sectors & (uint64_t)INT_MAX << 2;
| >       if (kind == IDE_CD) {
| > +        s->nb_sectors &= (uint64_t)INT_MAX << 2;
|
| 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.
| 
| ... it doesn't hurt either to have INT_MAX << 2.

Okay, will do.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
diff mbox series

Patch

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 */