Re: [PATCH 9/9] gnttab: don't silently truncate GFNs in compat setup-table handling

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 26/08/2021 11:15, Jan Beulich wrote:
> Returning back truncated frame numbers is unhelpful: Quite likely
> they're not owned by the domain (if it's PV), or we may misguide the
> guest into writing grant entries into a page that it actually uses for
> other purposes.
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> RFC: Arguably in the 32-bit PV case it may be necessary to instead put
>      in place an explicit address restriction when allocating
>      ->shared_raw[N]. This is currently implicit by alloc_xenheap_page()
>      only returning memory covered by the direct-map.

Yet another reason why having the grant table be Xen memory, rather than
guest memory, was a terrible idea.  Changing this is in consideration
for the encrypted vm work.

Its fine for now, but is fragile and liable to break for e.g. an
xmalloc() -> vmalloc() conversion, or when we get 5-level paging and the
directmap boundary moves above 16T.



> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -175,8 +175,15 @@ int compat_grant_table_op(unsigned int c
>                                   i < (_s_)->nr_frames; ++i ) \
>                      { \
>                          compat_pfn_t frame = (_s_)->frame_list.p[i]; \
> -                        if ( __copy_to_compat_offset((_d_)->frame_list, \
> -                                                     i, &frame, 1) ) \
> +                        if ( frame != (_s_)->frame_list.p[i] ) \
> +                        { \
> +                            if ( VALID_M2P((_s_)->frame_list.p[i]) ) \
> +                                (_s_)->status = GNTST_address_too_big; \
> +                            else \
> +                                frame |= 0x80000000U;\

Space before the \.  (This is one reason why I hate this style.  The
borderline illegibility makes it almost impossible to spot style problems.)

With the adjustment from the previous patch, you'll need a break in here.

But for !valid case, shouldn't we saturate to ~0u ?  I recall from the
migration work that various kernels disagree on what constitutes an
invalid MFN.

Then again, I can't see what legitimate case we'd have for a truncation
and an invalid M2P entry needing translating.

~Andrew

> +                        } \
> +                        else if ( __copy_to_compat_offset((_d_)->frame_list, 
> \
> +                                                          i, &frame, 1) ) \
>                              (_s_)->status = GNTST_bad_virt_addr; \
>                      } \
>                  } while (0)
>
>