[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH] iscsi: Cap block count from GET LBA STATUS (CVE-2020-1711)
Date: Fri, 24 Jan 2020 14:42:46 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2

On 1/24/20 2:39 PM, Kevin Wolf wrote:
Am 24.01.2020 um 11:48 hat Felipe Franciosi geschrieben:
On Jan 24, 2020, at 10:04 AM, Philippe Mathieu-Daudé <address@hidden> wrote:
On 1/23/20 11:58 PM, Peter Lieven wrote:
Am 23.01.2020 um 22:29 schrieb Felipe Franciosi <address@hidden>:
On Jan 23, 2020, at 5:46 PM, Philippe Mathieu-Daudé <address@hidden> wrote:
On 1/23/20 1:44 PM, Felipe Franciosi wrote:
When querying an iSCSI server for the provisioning status of blocks (via
GET LBA STATUS), Qemu only validates that the response descriptor zero's
LBA matches the one requested. Given the SCSI spec allows servers to
respond with the status of blocks beyond the end of the LUN, Qemu may
have its heap corrupted by clearing/setting too many bits at the end of
its allocmap for the LUN.
A malicious guest in control of the iSCSI server could carefully program
Qemu's heap (by selectively setting the bitmap) and then smash it.
This limits the number of bits that iscsi_co_block_status() will try to
update in the allocmap so it can't overflow the bitmap.

Please add:

Fixes: CVE-2020-1711 (title of CVE if possible)

I wasn't sure we had one yet. Kevin: can you do the needful in your branch?

I added the CVE number, though I don't have a title.


The description starts with "A malicious guest in control of the
iSCSI server ..." so asserting (and killing the VM) doesn't seem

assert() isn't an error check, but it means that we deem it impossible
for the assertion to fail. This would be the case because we fixed (in
this patch) the only code path that we think could cause the problem.

We would only add it to find other buggy code paths that we missed or
that are added later.

Correct. That's why I would have the proper checks (or "trim"s) closer
to where they were issued to fail sooner. What I meant is that if a
guest issues any operation that spans past the end of the drive, then
the operation stops there and an error is returned accordingly.

Guests can't issue operations that span past the end of the drive. They
would return an error befor the iscsi driver is even called.

The only reason why we get such a request here is because of an internal
call with BDRV_REQUEST_MAX_BYTES. Maybe this should actually be changed
and then iscsi_co_block_status() could assert that the request doesn't
span past the end of the drive.

This means nothing should ever try to touch these bitmaps out of
bounds. Nevertheless, and further to that, assert()s can be used
closer to where the bitmap is touched to catch programming errors.

I suppose the iSCSI protocol has some error to return for invalid

Which invalid you are referring to? From the initiator or the target?
AFAICT the problem is that the SCSI SPEC doesn't limit a target to
respond provisioning status past the (current) end of the LUN (either
because this was not deemed important to stress, was forgotten, or is
intentionally allowed).

In any case, we don't get an invalid request here. We are who made the
request. It's an unexpected response that we got.

Also shouldn't we report some warning in case of such invalid
request? So the management side can look at the 'malicious iSCSI

I think having the option to do so is a good idea. There are two cases
I can think of that you run into a "malicious" storage server:
1) Someone hacked your storage server
2) Your control plane allows your compute to connect to a user
provided storage service

Thinking as an admin, if I only allow storage servers I provide, then
I want to see such warnings. If I let people point the VMM to dodgy
servers, then I probably don't want the log spam.

For this reason, we generally don't log things for failed I/O requests.
If we wanted to introduce it, we'd better find a way to do so
consistently everywhere and not just in a single place with a one-off

I'm just suggesting to use error_report().

reply via email to

[Prev in Thread] Current Thread [Next in Thread]