Re: [Qemu-devel] [PATCH v2 16/17] ahci: Recompute cur_cmd on migrate post load

[John Snow <jsnow@redhat.com>]

On 02/10/2015 04:56 AM, Stefan Hajnoczi wrote:
> On Tue, Dec 16, 2014 at 08:36:06PM -0500, John Snow wrote:
>> When the AHCI HBA device is migrated, all of the information that
>> led to the request being created is stored in the AHCIDevice
>> structures, except for pointers into guest data where return
>> information needs to be stored.
>>
>> The "cur_cmd" field is usually responsible for this.
>>
>> To rebuild the cur_cmd pointer post-migration, we can utilize
>> the busy_slot index to figure out where the command header
>> we are still processing is.
>>
>> This allows a machine in a halted state from rerror=stop or
>> werror=stop to be migrated and resume operations without issue.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/ide/ahci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index c153228..8078d3e 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1373,6 +1373,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
>>            */
>>           if (ad->busy_slot == -1) {
>>               check_cmd(s, i);
>> +        } else {
>> +            /* We are in the middle of a command, and may need to access
>> +             * the command header in guest memory again. */
>> +            ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot];
>
> Where do we check that ad->busy_slot is within ad->lst[] bounds?
>
> If a malicious source sends a bogus value, this patch will lead to
> out-of-bounds accesses.
>
> Stefan
>

Good point. I'll recheck the series with this in mind.