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

[Felipe Franciosi]

> 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?
>>> 
>>>> Cc: address@hidden
>>> 
>>> Yeah, that's there.
>>> 
>>>> 
>>>>> Signed-off-by: Felipe Franciosi <address@hidden>
>>>>> Signed-off-by: Peter Turschmid <address@hidden>
>>>>> Signed-off-by: Raphael Norwitz <address@hidden>
>>>>> ---
>>>>> block/iscsi.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>>> index 2aea7e3f13..cbd57294ab 100644
>>>>> --- a/block/iscsi.c
>>>>> +++ b/block/iscsi.c
>>>>> @@ -701,7 +701,7 @@ static int coroutine_fn 
>>>>> iscsi_co_block_status(BlockDriverState *bs,
>>>>>     struct scsi_get_lba_status *lbas = NULL;
>>>>>     struct scsi_lba_status_descriptor *lbasd = NULL;
>>>>>     struct IscsiTask iTask;
>>>>> -    uint64_t lba;
>>>>> +    uint64_t lba, max_bytes;
>>>>>     int ret;
>>>>>       iscsi_co_init_iscsitask(iscsilun, &iTask);
>>>>> @@ -721,6 +721,7 @@ static int coroutine_fn 
>>>>> iscsi_co_block_status(BlockDriverState *bs,
>>>>>     }
>>>>>       lba = offset / iscsilun->block_size;
>>>>> +    max_bytes = (iscsilun->num_blocks - lba) * iscsilun->block_size;
>>>>>       qemu_mutex_lock(&iscsilun->mutex);
>>>>> retry:
>>>>> @@ -764,7 +765,7 @@ retry:
>>>>>         goto out_unlock;
>>>>>     }
>>>>> -    *pnum = (int64_t) lbasd->num_blocks * iscsilun->block_size;
>>>>> +    *pnum = MIN((int64_t) lbasd->num_blocks * iscsilun->block_size, 
>>>>> max_bytes);
>>>>>       if (lbasd->provisioning == SCSI_PROVISIONING_TYPE_DEALLOCATED ||
>>>>>         lbasd->provisioning == SCSI_PROVISIONING_TYPE_ANCHORED) {
>>>> 
>>>> What about this?
>>>> 
>>>> -- >8 --
>>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>>> index 2aea7e3f13..25598accbb 100644
>>>> --- a/block/iscsi.c
>>>> +++ b/block/iscsi.c
>>>> @@ -506,6 +506,11 @@ iscsi_allocmap_update(IscsiLun *iscsilun, int64_t 
>>>> offset,
>>>>    /* shrink to touch only completely contained clusters */
>>>>    cl_num_shrunk = DIV_ROUND_UP(offset, iscsilun->cluster_size);
>>>>    nb_cls_shrunk = (offset + bytes) / iscsilun->cluster_size - 
>>>> cl_num_shrunk;
>>>> +    if (nb_cls_expanded >= iscsilun->allocmap_size
>>>> +        || nb_cls_shrunk >= iscsilun->allocmap_size) {
>>>> +        error_report("iSCSI invalid request: ..." /* TODO */);
>>>> +        return;
>>>> +    }
>>>>    if (allocated) {
>>>>        bitmap_set(iscsilun->allocmap, cl_num_expanded, nb_cls_expanded);
>>>>    } else {
>>>> ---
>>> 
>>> I'm not sure the above is correct because (if I read this right)
>>> nb_cls_* represents the number of clusters, not the last cluster.
>>> 
>>> Personally, I would have the checks (or "trim"s) closer to where they
>>> were issued (to fail sooner) and assert()s closer to bitmap (as no oob
>>> accesses should be happening at this point). There were also
>>> discussions about using safer (higher level) bitmaps for this. I'm
>>> always in favour of adding all reasonable checks. :)
>> I would add assertions that cl_num + nb_cls <= allocmap_size before every 
>> set and clear.
> 
> The description starts with "A malicious guest in control of the iSCSI server 
> ..." so asserting (and killing the VM) doesn't seem correct...

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.

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 requests.

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).

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

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.

F.