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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Jan Beulich" <JBeulich@suse.com>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
	"Tamas K Lengyel" <tamas@tklengyel.com>
Subject: Re: [PATCH v8 06/16] xen/memory: Fix mapping grant tables with XENMEM_acquire_resource
Date: Mon, 1 Feb 2021 13:07:52 +0100	[thread overview]
Message-ID: <YBfvGKkQWSisFHNs@Air-de-Roger> (raw)
In-Reply-To: <53a26fb9-9c43-d1c4-90cd-bb29d57e106b@citrix.com>

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).

> >
> >>                  }
> >>              }
> >> @@ -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@citrix.com>

Thanks, Roger.