hw/ide/atapi.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)
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
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
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
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
>
+-- 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
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
+-- 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
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
+-- 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
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(-)
© 2016 - 2026 Red Hat, Inc.