From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hannes Reinecke <hare@suse.de>
Subject: Re: [Qemu-devel] [PATCH 5/5] megasas: LSI Megaraid SAS emulation
Date: Wed, 06 Jul 2011 08:20:58 +0200
Message-ID: <4E13FECA.10101@suse.de>
References: <1309863815-28236-1-git-send-email-hare@suse.de>	<1309863815-28236-2-git-send-email-hare@suse.de>	<1309863815-28236-3-git-send-email-hare@suse.de>	<1309863815-28236-4-git-send-email-hare@suse.de>	<1309863815-28236-5-git-send-email-hare@suse.de>	<1309863815-28236-6-git-send-email-hare@suse.de> <CAJSP0QVDh1+1Z2Z5h4d+7MsYTN17gNmuHvjONnmD1AzD=n4hvg@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
	format=flowed
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Stefan Haynoczi <stefanha@linux.vnet.ibm.com>,
	kvm@vger.kernel.org, Alexander Graf <agraf@suse.de>
To: Stefan Hajnoczi <stefanha@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from cantor2.suse.de ([195.135.220.15]:50817 "EHLO mx2.suse.de"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751622Ab1GFGVD (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 6 Jul 2011 02:21:03 -0400
In-Reply-To: <CAJSP0QVDh1+1Z2Z5h4d+7MsYTN17gNmuHvjONnmD1AzD=n4hvg@mail.gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 07/05/2011 05:21 PM, Stefan Hajnoczi wrote:
> On Tue, Jul 5, 2011 at 12:03 PM, Hannes Reinecke<hare@suse.de>  wrote=
:
>> +static void megasas_unmap_sgl(struct megasas_cmd_t *cmd)
>> +{
>> +    uint16_t flags =3D le16_to_cpu(cmd->frame->header.flags);
>> +    int i, is_write =3D (flags&  MFI_FRAME_DIR_WRITE) ? 1 : 0;
>> +
>> +    for (i =3D 0; i<  cmd->frame->header.sge_count; i++) {
>> +        cpu_physical_memory_unmap(cmd->iov[i].iov_base, cmd->iov[i]=
=2Eiov_len,
>> +                                  is_write, cmd->iov[i].iov_len);
>> +    }
>
> We cannot map control structures from guest memory and treating them
> as valid request state later on.
>
Yes, I've been working on that one already.
What I'll be doing is to read in the sge count during 'map_sgl' and=20
store this value internally (in ->iov_cnt). And during unmap I'll be=20
using this value instead of the frame-provided one.

That way we'll be checking the sge_count field only once when we=20
slurp in the entire frame.

> A malicious guest can issue the request, then change the fields the
> control structure while QEMU is processing the I/O, and then this
> function will execute with is_write/sge_count no longer the same as
> when the request started.
>
> Good practice would be to copy in any request state needed instead of
> reaching into guest memory at later points of the request lifecycle.
> This way a malicious guest can never cause QEMU to crash or do
> something due to inconsistent state.
>
See above, that's what I'll be doing.

> The particular problem I see here is starting the request with
> sge_count=3D1 and then setting it to sge_count=3D255.  We will perfor=
m
> invalid iov[] accesses.
>

Thanks for the hint. Will be fixing it up.

Cheers,

Hannes
--=20
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg
GF: J. Hawn, J. Guild, F. Imend=C3=B6rffer, HRB 16746 (AG N=C3=BCrnberg=
)