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
