Re: [Qemu-devel] [PATCH 5/5] megasas: LSI Megaraid SAS emulation

[Hannes Reinecke <hare@suse.de>]

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 = le16_to_cpu(cmd->frame->header.flags);
>> +    int i, is_write = (flags&  MFI_FRAME_DIR_WRITE) ? 1 : 0;
>> +
>> +    for (i = 0; i<  cmd->frame->header.sge_count; i++) {
>> +        cpu_physical_memory_unmap(cmd->iov[i].iov_base, cmd->iov[i].iov_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 
store this value internally (in ->iov_cnt). And during unmap I'll be 
using this value instead of the frame-provided one.

That way we'll be checking the sge_count field only once when we 
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=1 and then setting it to sge_count=255.  We will perform
> invalid iov[] accesses.
>

Thanks for the hint. Will be fixing it up.

Cheers,

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