Re: [PATCH v2 07/11] xen/memory: Improve compat XENMEM_acquire_resource handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.09.2020 20:24, Andrew Cooper wrote:
> @@ -446,6 +430,31 @@ int compat_memory_op(unsigned int cmd, 
> XEN_GUEST_HANDLE_PARAM(void) compat)
>  
>  #undef XLAT_mem_acquire_resource_HNDL_frame_list
>  
> +            if ( xen_frame_list && cmp.mar.nr_frames )
> +            {
> +                /*
> +                 * frame_list is an input for translated guests, and an 
> output
> +                 * for untranslated guests.  Only copy in for translated 
> guests.
> +                 */
> +                if ( paging_mode_translate(currd) )
> +                {
> +                    compat_pfn_t *compat_frame_list = (void *)xen_frame_list;
> +
> +                    if ( !compat_handle_okay(cmp.mar.frame_list,
> +                                             cmp.mar.nr_frames) ||
> +                         __copy_from_compat_offset(
> +                             compat_frame_list, cmp.mar.frame_list,
> +                             0, cmp.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 )
> +                        xen_frame_list[x] = compat_frame_list[x];

In addition to what Paul has said, I also don't see why you resort
to a signed type here. Using the available local variable i ought to
be quite easy:

                    for ( i = cmp.mar.nr_frames; i--; )
                        xen_frame_list[i] = compat_frame_list[i];

As an aside, considering the controversy we're having on patch 2, I
find it quite interesting how you carefully allow for nr_frames being
zero throughout your changes here (which, as I think is obvious, I
agree you want to do).

Jan