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@suse.com>
---
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.
--- 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;\
+ } \
+ else if ( __copy_to_compat_offset((_d_)->frame_list, \
+ i, &frame, 1) ) \
(_s_)->status = GNTST_bad_virt_addr; \
} \
} while (0)
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@suse.com>
> ---
> 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)
>
>
On 07.10.2022 21:57, Andrew Cooper wrote:
> On 26/08/2021 11:15, Jan Beulich wrote:
>> --- 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.)
There is a (imo severe) downsides to backslashes on the far right as well:
It's easier to miss adding one on a newly added line, which may or may not
result in a build failure.
> With the adjustment from the previous patch, you'll need a break in here.
Can do. Question then is whether to go further right here and adjust
the loop header and the other setting of (_s_)->status at the same time.
> 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.
I've dropped the use of VALID_M2P(). It's a bogus check anyway (I don't
actually recall how I came to think of doing this sort of check), and
there's indeed no reason to report back an (overflow) error in this way.
Furthermore I've noticed that the updating of "frame" was actually dead
code - the updated variable wasn't really used for anything; we would
have left both ->status and the array slot untouched, misguiding the
caller.
Jan
© 2016 - 2026 Red Hat, Inc.