Re: [XEN][PATCH v3] x86: make Viridian support optional

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.09.2025 19:17, Grygorii Strashko wrote:
> On 18.09.25 18:41, Jan Beulich wrote:
>> On 16.09.2025 15:41, Grygorii Strashko wrote:
>>> --- a/xen/arch/x86/hvm/viridian/viridian.c
>>> +++ b/xen/arch/x86/hvm/viridian/viridian.c
>>> @@ -1116,14 +1116,14 @@ static int cf_check viridian_save_domain_ctxt(
>>>   {
>>>       const struct domain *d = v->domain;
>>>       const struct viridian_domain *vd = d->arch.hvm.viridian;
>>> -    struct hvm_viridian_domain_context ctxt = {
>>> -        .hypercall_gpa = vd->hypercall_gpa.raw,
>>> -        .guest_os_id = vd->guest_os_id.raw,
>>> -    };
>>> +    struct hvm_viridian_domain_context ctxt = {};
>>>   
>>>       if ( !is_viridian_domain(d) )
>>>           return 0;
>>
>> This check doesn't check for vd being non-NULL, so this still feels a little
>> fragile, even if it looks correct now.
> 
> Hm. May be I missing smth., but
> - if is_viridian_domain(d) and viridian_domain_init() succeeded
>    then d->arch.hvm.viridian != NULL, like always
>    (otherwise domain will not be created)
> 
> - if !is_viridian_domain() then code will not go further
> 
> so I'm missing to see how !d->arch.hvm.viridian (!vd) can happen here.

Well, as said - it feels a _little_ fragile, as it's not the NULL-ness of
the pointer that is checked.

>>> +    ctxt.hypercall_gpa = vd->hypercall_gpa.raw;
>>> +    ctxt.guest_os_id = vd->guest_os_id.raw,
>>> +
>>>       viridian_time_save_domain_ctxt(d, &ctxt);
>>>       viridian_synic_save_domain_ctxt(d, &ctxt);
>>>   
>>
>> Just below here we have viridian_load_domain_ctxt(), which I'm pretty sure
>> now also needs to gain some check: Save records coming from user space, we
>> can't really rely on there being none of this type for a non-Viridian domain.
> 
> As per my understanding:
> viridian_load_domain_ctxt() calls hvm_load_entry_zeroextend() which
> should not succeed if context was not saved (which shouldn't happen for
> !is_viridian_domain(d) case).

I don't understand what you mean with "was not saved". To be free of security
issues, we need to cope with any (bogus or not) record appearing.

Jan