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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

~Andrew

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