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

[Scott Davis <scottwd@xxxxxxxxx>]

Thanks for the review, Jason!

On Tue, Aug 17, 2021 at 9:15 PM Jason Andryuk <jandryuk@xxxxxxxxx> wrote:
>> 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"?

Ack for v3.

>> - 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.

Ack for v3.

>> - 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.

As you said, there are advantages to both approaches. Relabeling the
stubdom before its launch would eliminate potential race conditions
between stubdom init and the run-time relabeling, which could cause
sporadic failures in any init steps that can fail to complete before
the relabel. On the other hand, waiting to relabel allows any
(dom0 -> stubdom) privileges that are required only for stubdom init,
particularly device passthrough setup, to be dropped from the run-time
label. (stubdom/dom0 -> domU) privileges are of less concern, since
they can be dropped by relabeling of the guest regardless.

At this point I can neither rule out synchronization issues with the late
relabeling approach nor quantify the benefits, as the policy I've been
using to verify this is fairly maximal in terms of run-time privileges.
I do intend to spend some time experimenting and shrinking it down in
the near future, though. Before that experimentation, I'll add a second
stubdom relabel point just before the stubdom is unpaused, along with a
config boolean to select which relabel point is used. That should allow
me to determine how much security benefit we'd be getting from the
later relabeling, and may be how things should go in upstream so that
users have a choice of approach.

In the meantime, input from any other devs is certainly welcome.

Good day,
    Scott