Re: [PATCH v8 06/16] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.pau@xxxxxxxxxx>

Thanks.

~Andrew