Re: [PATCH v3 2/2] xen: introduce CONFIG_HAS_SHARED_INFO for archs without a shared page

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 6/17/26 4:24 PM, Jan Beulich wrote:
> On 17.06.2026 16:02, Oleksii Kurochko wrote:
> > On 6/17/26 3:26 PM, Jan Beulich wrote:
> > > On 03.06.2026 16:25, Oleksii Kurochko wrote:
> > > > --- a/xen/common/domain.c
> > > > +++ b/xen/common/domain.c
> > > > @@ -320,9 +320,9 @@ void vcpu_info_reset(struct vcpu *v)
> > > >        struct domain *d = v->domain;
> > > >    
> > > >        v->vcpu_info_area.map =
> > > > -        ((v->vcpu_id < XEN_LEGACY_MAX_VCPUS)
> > > > -         ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> > > > -         : &dummy_vcpu_info);
> > > > +        IS_ENABLED(CONFIG_HAS_SHARED_INFO) && v->vcpu_id < XEN_LEGACY_MAX_VCPUS
> > > > +        ? (vcpu_info_t *)&shared_info(d, vcpu_info[v->vcpu_id])
> > > > +        : &dummy_vcpu_info;
> > > >    }
> > > While the change here is likely okay, it points at possible further omissions.
> > > You've dealt with all uses of shared_info(), but you've left alone all uses of
> > > vcpu_info() (and __vcpu_info()). Reads are presumably okay, but writes to
> > > dummy_vcpu_info open a side channel for possible info leaks. Looking over uses
> > > in common code, no code changes may be needed; extending the description may
> > > be all that's wanted here.
> > Isn't there already a side channel that could allow leaks, even without
> > this change?
> There are multiple aspects here. First, for PV secondary vCPU-s cannot be
> launched when their vcpu-info still points at dummy_vcpu_info. HVM vCPU-s
> make very limited use of vcpu-info fields. Writes look to be limited to
> the evtchn_upcall_{mask,pending} fields, which isn't really an info leak.
>
> My main point here is: None of this goes without making clear that the
> necessary auditing was properly done.
>
> > The change here just made it worsen because now info leak
> > will happen for all vCPUs when  CONFIG_HAS_SHARED_INFO=n.
> >
> > I will add to the description the following:
> > ```
> > With CONFIG_HAS_SHARED_INFO=n all vCPUs fall back to the global
> > dummy_vcpu_info, so writes through vcpu_info() could leak data between
> > vCPUs.  Reviewing the write paths in common code: the write in
> > map_guest_area() stores the constant ~0 so nothing serious will happen
> > if it will be leaked; the event_2l.c paths are unreachable because the
> > preceding shared_info() call would trap first; the write in
> > vcpu_info_populate() targets the new mapping buffer, not
> > dummy_vcpu_info; all remaining writes are x86 PV-specific for which
> > CONFIG_HAS_SHARED_INFO=y.  No code changes are needed.
> > ```
> As you start with "common code", how come the "x86 PV-specific" part is
> still there (i.e. relevant)? Isn't all PV stuff in x86-specific code?
Good point. The "x86 PV-specific" part is not part of the review of 
common code. I mentioned it separately to complete the audit of all 
write paths reachable through vcpu_info(). The intent was:
* the writes discussed before the semicolon are the common-code paths;
* the writes after the semicolon are outside common code and are x86 
PV-specific, where CONFIG_HAS_SHARED_INFO=y anyway.
To avoid the ambiguity, I can reword the sentence to make that 
separation explicit:
```
With CONFIG_HAS_SHARED_INFO=n all vCPUs fall back to the global
dummy_vcpu_info, so writes through vcpu_info() could leak data between
vCPUs. Reviewing the write paths in common code: the write in
map_guest_area() stores the constant ~0 so nothing serious would happen
if it were leaked; the event_2l.c paths are unreachable because the
preceding shared_info() call would trap first; the write in
vcpu_info_populate() targets the new mapping buffer, not
dummy_vcpu_info.

Outside common code, the remaining writes are x86 PV-specific, for which
CONFIG_HAS_SHARED_INFO=y. No code changes are needed.
```

Would that wording work for you?

Thanks.

~ Oleksii