On 01/02/2021 12:07, Roger Pau Monné wrote:
> On Mon, Feb 01, 2021 at 11:11:37AM +0000, Andrew Cooper wrote:
>> On 01/02/2021 10:10, Roger Pau Monné wrote:
>>> On Sat, Jan 30, 2021 at 02:58:42AM +0000, Andrew Cooper wrote:
>>>> + (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.mar)) /
>>>> + sizeof(*xen_frame_list);
>>>> +
>>>> + if ( start_extent >= cmp.mar.nr_frames )
>>>> + return -EINVAL;
>>>> +
>>>> + /*
>>>> + * Adjust nat to account for work done on previous
>>>> + * continuations, leaving cmp pristine. Hide the
>>>> continaution
>>>> + * from the native code to prevent double accounting.
>>>> + */
>>>> + nat.mar->nr_frames -= start_extent;
>>>> + nat.mar->frame += start_extent;
>>>> + cmd &= MEMOP_CMD_MASK;
>>>> +
>>>> + /*
>>>> + * If there are two many frames to fit within the xlat
>>>> buffer,
>>>> + * we'll need to loop to marshal them all.
>>>> + */
>>>> + nat.mar->nr_frames = min(nat.mar->nr_frames,
>>>> xlat_max_frames);
>>>> +
>>>> /*
>>>> * frame_list is an input for translated guests, and an
>>>> output
>>>> * for untranslated guests. Only copy in for translated
>>>> guests.
>>>> @@ -444,14 +453,14 @@ int compat_memory_op(unsigned int cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>> cmp.mar.nr_frames) ||
>>>> __copy_from_compat_offset(
>>>> compat_frame_list, cmp.mar.frame_list,
>>>> - 0, cmp.mar.nr_frames) )
>>>> + start_extent, nat.mar->nr_frames) )
>>>> return -EFAULT;
>>>>
>>>> /*
>>>> * Iterate backwards over compat_frame_list[]
>>>> expanding
>>>> * compat_pfn_t to xen_pfn_t in place.
>>>> */
>>>> - for ( int x = cmp.mar.nr_frames - 1; x >= 0; --x )
>>>> + for ( int x = nat.mar->nr_frames - 1; x >= 0; --x )
>>>> xen_frame_list[x] = compat_frame_list[x];
>>> Unrelated question, but I don't really see the point of iterating
>>> backwards, wouldn't it be easy to use use the existing 'i' loop
>>> counter and for a for ( i = 0; i < nat.mar->nr_frames; i++ )?
>>>
>>> (Not that you need to fix it here, just curious about why we use that
>>> construct instead).
>> Iterating backwards is totally critical.
>>
>> xen_frame_list and compat_frame_list are the same numerical pointer.
>> We've just filled it 50% full with compat_pfn_t's, and need to turn
>> these into xen_pfn_t's which are double the size.
>>
>> Iterating forwards would clobber every entry but the first.
> Oh, I didn't realize they point to the same address. A comment would
> help (not that you need to add it now).
Well - that's what "expand ... in place" means in the existing comment.
Suggestions for how to make it clearer?
>
>>>> }
>>>> }
>>>> @@ -600,9 +609,11 @@ int compat_memory_op(unsigned int cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>> case XENMEM_acquire_resource:
>>>> {
>>>> DEFINE_XEN_GUEST_HANDLE(compat_mem_acquire_resource_t);
>>>> + unsigned int done;
>>>>
>>>> if ( compat_handle_is_null(cmp.mar.frame_list) )
>>>> {
>>>> + ASSERT(split == 0 && rc == 0);
>>>> if ( __copy_field_to_guest(
>>>> guest_handle_cast(compat,
>>>> compat_mem_acquire_resource_t),
>>>> @@ -611,6 +622,21 @@ int compat_memory_op(unsigned int cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>> break;
>>>> }
>>>>
>>>> + if ( split < 0 )
>>>> + {
>>>> + /* Continuation occurred. */
>>>> + ASSERT(rc != XENMEM_acquire_resource);
>>>> + done = cmd >> MEMOP_EXTENT_SHIFT;
>>>> + }
>>>> + else
>>>> + {
>>>> + /* No continuation. */
>>>> + ASSERT(rc == 0);
>>>> + done = nat.mar->nr_frames;
>>>> + }
>>>> +
>>>> + ASSERT(done <= nat.mar->nr_frames);
>>>> +
>>>> /*
>>>> * frame_list is an input for translated guests, and an
>>>> output for
>>>> * untranslated guests. Only copy out for untranslated
>>>> guests.
>>>> @@ -626,7 +652,7 @@ int compat_memory_op(unsigned int cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>> */
>>>> BUILD_BUG_ON(sizeof(compat_pfn_t) > sizeof(xen_pfn_t));
>>>>
>>>> - for ( i = 0; i < cmp.mar.nr_frames; i++ )
>>>> + for ( i = 0; i < done; i++ )
>>>> {
>>>> compat_pfn_t frame = xen_frame_list[i];
>>>>
>>>> @@ -636,15 +662,45 @@ int compat_memory_op(unsigned int cmd,
>>>> XEN_GUEST_HANDLE_PARAM(void) compat)
>>>> compat_frame_list[i] = frame;
>>>> }
>>>>
>>>> - if ( __copy_to_compat_offset(cmp.mar.frame_list, 0,
>>>> + if ( __copy_to_compat_offset(cmp.mar.frame_list,
>>>> start_extent,
>>>> compat_frame_list,
>>>> - cmp.mar.nr_frames) )
>>>> + done) )
>>>> return -EFAULT;
>>> Is it fine to return with a possibly pending continuation already
>>> encoded here?
>>>
>>> Other places seem to crash the domain when that happens.
>> Hmm. This is all a total mess. (Elsewhere the error handling is also
>> broken - a caller who receives an error can't figure out how to recover)
>>
>> But yes - I think you're right - the only thing we can do here is `goto
>> crash;` and woe betide any 32bit kernel which passes a pointer to a
>> read-only buffer.
> With that added:
>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
Thanks.
~Andrew
