Re: [PATCH v7 17/22] x86/virt/tdx: Avoid updates during update-sensitive operations

[Dan Williams <djbw@kernel.org>]

Edgecombe, Rick P wrote:
[..]
> > If an update races TD build, for example, TD measurement can become
> > incorrect and attestation can fail.
> > 
> > The TDX architecture exposes two approaches:
> > 
> > 1) Avoid updates during update-sensitive operations.
> > 2) Detect incompatibility after update and recover.
> > 
> > Post-update detection (option #2) is not a good fit: as discussed in [1],
> > future module behavior may expand update-sensitive operations in ways that
> > make KVM ABIs unstable and will break userspace.
> > 
> > "Do nothing" is also not preferred: while it keeps kernel code simple, it
> > lets the issue leak into the broader stack, where both detection and
> > recovery require significantly more effort.
> 
> This subject has had a lot of debate (as linked below in the log), but the way
> this is written leaves a lot of questions. "do nothing" is not an option it
> says, but the code does just that when UPDATE_COMPAT_SENSITIVE is not supported.

Right, but that is not the kernel's problem. If you run updates on
a module that does not support conflict detection, you get to keep the
pieces.

Like other cases of Linux not wanting to deal with the eccentricities of
early modules, just have userspace know about a minimum module version
where this protocol exists and accept the risk otherwise. No pressing
need to burden the kernel with carrying worry for early modules.

> > So, use option #1. Specifically, request "avoid update-sensitive" behavior
> > during TDX module shutdown and map the resulting failure to -EBUSY so
> > userspace can distinguish an update race from other failures.
> > 
> > When the "avoid update-sensitive" feature isn't supported, proceed with
> > updates. If a race occurs between module update and update-sensitive
> > operations, failures happen at a later stage (e.g., incorrect TD
> > measurements in attestation reports for TD build). Effectively, this
> > means "let userspace update at their own risk".
> > 
> 
> Above it says we can't just do nothing, we need the flag. And then this argues
> that we can do nothing because we can rely on userspace to deal with the
> issue... This log is maybe just trying to put a brave face on an imperfect
> compromise?

Not really, the log could be simplified to just say "module versions < X
are not safe for runtime update". In general if the kernel had a minimum
supported module version concept it could collect all of these early
deprecation conditions under one check.

> So, while I don't want to re-open the debate, I'm not sure the patch
> justification is going to pass scrutiny as is.
> 
> In the link [2], Dan says "Do not make Linux carry short lived one-off
> complexity", and also "Do not include logic to disable updates, document the
> expectation in the tool."
> 
> It seems this does not exclude the option to just to always pass the compat
> flag. Basically assume that the TDX module will always support
> UPDATE_COMPAT_SENSITIVE if it supports TDX module updates. Which I guess we
> should expect should eventually be true.

Right, assume a minimum module version.

> In [2] Dan was also against checking the UPDATE_COMPAT_SENSITIVE feature0 bit to
> gate the feature.

...because if it must be supported, why check?

> For the record, I don't like allowing the update without the compat bit set, and
> my concern has nothing to do with userspace roles and responsibilities. Instead
> it's because we are over budget on complexity for handling SEAMCALL errors
> within KVM and this makes things worse to keep track of.
> 
> tdh_mem_page_add() does a KVM_BUG_ON() if it sees a non-busy error. Imagine
> working on this code and considering if it is a valid KVM_BUG_ON()? After this
> patch, the answer is...well sometimes. It depends on the previous modules
> specific feature0 bits, an understanding on admins expectations, and the
> behavior of some far away code in arch/x86. Gah.

Why would it be variable? The user tried update on a module that the
kernel deemed unfit for update. "Doctor, it KVM_BUG_ON()s when I run
update".

> Actually, the diff Dan objected to was checking and printing a specific helpful
> error. Maybe he does not mind much more simply checking an extra bit in
> tdx_supports_runtime_update()? Otherwise, I'd think to just unconditionally pass
> UPDATE_COMPAT_SENSITIVE without checking for support. Essentially mandate that
> it is always supported if TDX module update is supported.

In the end all of the hand wringing we are doing is misplaced. The root
of the problem is the TDX Module claiming "runtime update supported" to
include these corner cases of "but we corrupt things if the update tries
to replace the crypto library at the wrong time".

That problem is also solvable by not classifying those updates as
runtime compatible, or defining a protocol to mitigate the collisions.

Neither of those was chosen. Instead the module implemented 2 additional
complications Linux chose one that fails update, other VMMs chose one
that maximizes updates at the cost of needing to restart TD
construction.

I think the changelog is a bit non-commital trying to be diplomatic
about the mess. Simply, Linux wanted the easy button, all runtime
updates are safe. Instead, module exported complexity and optionality.
KVM voted for one flavor of that optionality to accommodate the module
complexity.

The solution, make modules with the option KVM wants the min requirement
and move on.