Re: [Xen-devel] [PATCH V2] xen/hvm: fix hypervisor crash with hvm_save_one()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.17 at 14:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/05/17 11:44, Razvan Cojocaru wrote:
>> On 05/03/17 12:30, Jan Beulich wrote:
>>>>>> On 03.05.17 at 11:21, <tim@xxxxxxx> wrote:
>>>> At 10:15 +0100 on 03 May (1493806508), Tim Deegan wrote:
>>>>> At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote:
>>>>>> +    else if ( ctxt.cur > sizeof(*desc) )
>>>>>>      {
>>>>>>          uint32_t off;
>>>>>> -        const struct hvm_save_descriptor *desc;
>>>>>>  
>>>>>> -        rv = -ENOENT;
>>>>>>          for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += 
>>>>>> desc->length 
> )
>>>> It occurs to me that as well as underflowing, this test is off by one.
>>>> It ought to be "off + sizeof(*desc) <= ctxt.cur" to allow for a
>>>> zero-length record.  AFAIK we don't actually have any of those, so
>>>> it's academic, but we might want to represent the presence of some
>>>> feature without having any feature-specific state to save.
>>> Good point; I already have two follow-up patches, one of which I
>>> think this adjustment would easily fit into.
>> Should I re-send the original patch with the updated comment then (thus
>> also being able to keep Andrew's Signed-off-by), and if so, is it
>> alright to keep Julien's Release-Acked-by?
>>
>> Or will you use this later patch (presumably with your Signed-off-by),
>> in which case I should test it?
>>
>> Things got rather confusing (apologies for my own part in the confusion).
> 
> I am not opposed to Jan's alternative, but I think we should make the
> adjustment to cope with zero length records.

I can certainly move this here from the follow-up patch.

> At this point, it might be best for Jan to submit a complete patch and
> for Razvan to double check that it still resolves the issue.

Let me do that then.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel