On 11.11.2025 18:52, Grygorii Strashko wrote:
> On 10.11.25 09:11, Jan Beulich wrote:
>> On 07.11.2025 19:17, Grygorii Strashko wrote:
>>> --- a/xen/arch/x86/include/asm/guest_access.h
>>> +++ b/xen/arch/x86/include/asm/guest_access.h
>>> @@ -13,26 +13,64 @@
>>> #include <asm/hvm/guest_access.h>
>>> /* Raw access functions: no type checking. */
>>> -#define raw_copy_to_guest(dst, src, len) \
>>> - (is_hvm_vcpu(current) ? \
>>> - copy_to_user_hvm((dst), (src), (len)) : \
>>> - copy_to_guest_pv(dst, src, len))
>>> -#define raw_copy_from_guest(dst, src, len) \
>>> - (is_hvm_vcpu(current) ? \
>>> - copy_from_user_hvm((dst), (src), (len)) : \
>>> - copy_from_guest_pv(dst, src, len))
>>> -#define raw_clear_guest(dst, len) \
>>> - (is_hvm_vcpu(current) ? \
>>> - clear_user_hvm((dst), (len)) : \
>>> - clear_guest_pv(dst, len))
>>> -#define __raw_copy_to_guest(dst, src, len) \
>>> - (is_hvm_vcpu(current) ? \
>>> - copy_to_user_hvm((dst), (src), (len)) : \
>>> - __copy_to_guest_pv(dst, src, len))
>>> -#define __raw_copy_from_guest(dst, src, len) \
>>> - (is_hvm_vcpu(current) ? \
>>> - copy_from_user_hvm((dst), (src), (len)) : \
>>> - __copy_from_guest_pv(dst, src, len))
>>> +static inline bool raw_use_hvm_access(const struct vcpu *v)
>>> +{
>>> + return IS_ENABLED(CONFIG_HVM) && (!IS_ENABLED(CONFIG_PV) ||
>>> is_hvm_vcpu(v));
>>> +}
>>
>> Without a full audit (likely tedious and error prone) this still is a
>> behavioral change for some (likely unintended) use against a system domain
>> (likely the idle one): With HVM=y PV=n we'd suddenly use the HVM accessor
>> there. IOW imo the "system domains are implicitly PV" aspect wants
>> retaining, even if only "just in case". It's okay not to invoke the PV
>> accessor (but return "len" instead), but it's not okay to invoke the HVM
>> one.
>
> This patch is subset of "constify is_hvm_domain() for PV=n case" attempts.
>
> It was made under assumption that:
> "System domains do not have Guests running, so can't initiate hypecalls and
> can not be users of copy_to/from_user() routines. There are no Guest and no
> user memory".
> [IDLE, COW, IO, XEN]
>
> If above assumption is correct - this patch was assumed safe.
>
> if not - it all make no sense, probably.
I wouldn't go as far as saying that. It can be arranged to avid the corner
case I mentioned, I think.
Jan
