Re: [PATCH v2] tools/xl: Add device_model_stubdomain_init_seclabel option to xl.cfg

[Jason Andryuk <jandryuk@xxxxxxxxx>]

Hi, Scott

On Fri, Jul 30, 2021 at 3:35 PM Scott Davis <scottwd@xxxxxxxxx> wrote:
>
> This adds an option to the xl domain configuration syntax for specifying
> a build-time XSM security label for device-model stubdomains separate
> from the run-time label specified by 'device_model_stubdomain_seclabel'.
> Fields are also added to the 'libxl_domain_build_info' struct to contain
> the new information, and a new call to 'xc_flask_relabel_domain'
> inserted to affect the change at the appropriate time.
>
> The device-model stubdomain is relabeled at the same time as its guest,
> just before the guest is unpaused. This requires the stubdomain itself
> to be unpaused and run for a short time prior to being relabeled, but
> allows PCI device assignments specified in xl.cfg to be completed prior
> to relabeling. This choice allows the privileges required to perform
> assignments to be dropped in the relabeling.
>
> The implementation mirrors that of the 'seclabel' and 'init_seclabel'
> options for user domains. When all used in concert, this enables the

Drop all -> "When used in concert"?

> creation of security policies that minimize run-time privileges between
> the toolstack domain, device-model stubdomains, and user domains.
>
> Signed-off-by: Scott Davis <scott.davis@xxxxxxxxxx>
> ---
> Changes in v2:
> - removed superfluous chanegs to libxl_dm.c
> - changed all security label lookup failures due to FLASK being disabled
>   from warnings to hard errors based on mailing list discussion

I think this should be a separate patch before this one, since it is
distinct from adding device_model_stubdomain_init_seclabel.

> - added discussion of relabel point to commit message

I was hoping for more people's thoughts on this.  I'm just going to
write down my thoughts on this to get them out there.

The non-stubdom case is very straightforward:
<-init-><-relabel-><-unpause-><-exec->

Relabeling before unpause is a nice delineation point.  Since the
guest hasn't run, it can't be mid-operation when the relabel happens.

The stubdom case as implemented by the patch is this:
Guest:
<---------init------------->|<-relabel-><-unpause-><-exec->
Stubdom:                    |
<-init--><-unpause-><-exec->|<-relabel-><-exec->

Here the stubdom runs for some time and then gets relabeled.

The alternative would be to relabel the stubdom before unpause and
then relabel the guest:
Guest:
<----------init---------------------->|<-relabel-><-unpause-><-exec->
Stubdom:                              |
<-init-><-relabel-><-unpause-><-exec--|--exec->

Here relabel then unpause is maintained for both guest and stubdom.

Relabeling the *running* stubdom gives me pause.  We don't have any
synchronization with the stubdom's & QEMU's state.  QEMU wrote
"running" to indicate it was up, but that doesn't say anything about
the rest of the kernel.  From the other side, the stubdom and QEMU
running and having the guest label change could be surprising.

Scott, you are using this, so I may be imagining concerns that don't
exist.  And you did mention your approach gives the PCI assignment
only to the stubdom init label, which could be a nice feature.  It's
just the lack of clear delineation for the stubdom that makes me
wonder about it.

Having said that, for your approach, I think the code looks good.

Regards,
Jason