Re: [PATCH] domain: Validate __copy_to_guest in VCPUOP_register_runstate_memory_area

[Michal Orzel <michal.orzel@xxxxxxx>]

On 26/11/2024 11:40, Jan Beulich wrote:
> 
> 
> On 26.11.2024 11:26, Michal Orzel wrote:
>> For guests with paging mode external, guest_handle_okay() always returns
>> success, even if the guest handle is invalid (e.g. address not in P2M).
>> In VCPUOP_register_runstate_memory_area, we would then blindly set
>> runstate_guest() for a given vCPU to invalid handle. Moreover, we don't
>> check the return value from __copy_to_guest() and return success to the
>> guest, even in case of a failure during copy.
> 
> I'm afraid this is all deliberate, providing best effort behavior. For a
> paging mode external guest, the handle may become valid subsequently. If
> any __copy_to_guest() fail here, subsequent update_runstate_area() may
> succeed (and success of the actual copying isn't checked there either).
Hmm, I've never looked at that this way. I always thought that mapping must be 
in place
before setting up handle. Is this concept really something used (e.g. in Linux) 
or just a
way for us to provide excuse for not hardening the code because of a fear of 
regressions, etc.?

> 
>> Fix it, by checking the
>> return value from __copy_to_guest() and set runstate_guest() only on
>> success.
> 
> _If_ such a change was wanted (despite its potential for regressions,
> as guests may leverage present behavior to establish handles before
> putting in place mappings), x86'es compat_vcpu_op() would need updating,
> too. Plus what about VCPUOP_register_vcpu_time_memory_area, behaving
> similarly?
It's up to us. If the concept you mentioned is valid and people are aware of 
it, then
let's keep as is. I for one was amazed that Xen returned success but structure 
was not
copied.

~Michal