Re: [PATCH] hyperv: Add CONFIG_MSHV_ROOT to gate hv_root_partition checks

[Nuno Das Neves <nunodasneves@linux.microsoft.com>]

On 2/11/2025 9:47 PM, Easwar Hariharan wrote:
> On 2/11/2025 2:21 PM, Nuno Das Neves wrote:
>> Introduce CONFIG_MSHV_ROOT as a tristate to enable root partition
>> booting and future mshv driver functionality.
>>
>> Change hv_root_partition into a function which always returns false
>> if CONFIG_MSHV_ROOT=n.
>>
>> Introduce hv_current_partition_type to store the type of partition
>> (guest, root, or other kinds in future), and hv_identify_partition_type()
>> to it up early in Hyper-V initialization.
> 
> ...to *set* it up early?
> 
Yep! Thanks for catching that

>>
>> Signed-off-by: Nuno Das Neves <nunodasneves@linux.microsoft.com>
>> ---
>> Depends on
>> https://lore.kernel.org/linux-hyperv/1738955002-20821-3-git-send-email-nunodasneves@linux.microsoft.com/
>>
>>  arch/arm64/hyperv/mshyperv.c       |  2 ++
>>  arch/x86/hyperv/hv_init.c          | 10 ++++----
>>  arch/x86/kernel/cpu/mshyperv.c     | 24 ++----------------
>>  drivers/clocksource/hyperv_timer.c |  4 +--
>>  drivers/hv/Kconfig                 | 12 +++++++++
>>  drivers/hv/Makefile                |  3 ++-
>>  drivers/hv/hv.c                    | 10 ++++----
>>  drivers/hv/hv_common.c             | 32 +++++++++++++++++++-----
>>  drivers/hv/vmbus_drv.c             |  2 +-
>>  drivers/iommu/hyperv-iommu.c       |  4 +--
>>  include/asm-generic/mshyperv.h     | 39 +++++++++++++++++++++++++-----
>>  11 files changed, 92 insertions(+), 50 deletions(-)
>>
> 
> <snip>
> 
>> +
>> +void hv_identify_partition_type(void)
>> +{
>> +	/*
>> +	 * Check partition creation and cpu management privileges
>> +	 *
>> +	 * Hyper-V should never specify running as root and as a Confidential
>> +	 * VM. But to protect against a compromised/malicious Hyper-V trying
>> +	 * to exploit root behavior to expose Confidential VM memory, ignore
>> +	 * the root partition setting if also a Confidential VM.
>> +	 */
>> +	if ((ms_hyperv.priv_high & HV_CREATE_PARTITIONS) &&
>> +	    (ms_hyperv.priv_high & HV_CPU_MANAGEMENT) &&
>> +	    !(ms_hyperv.priv_high & HV_ISOLATION)) {
>> +		hv_current_partition_type = HV_PARTITION_TYPE_ROOT;
>> +		pr_info("Hyper-V: running as root partition\n");
>> +	} else {
>> +		hv_current_partition_type = HV_PARTITION_TYPE_GUEST;
>> +	}
>> +}
> 
> This should assume GUEST as default and modify to ROOT if all the checks pass.
> 
It is doing that, isn't it?

In fact the 'else' branch here is redundant and just there for additional clarity.

hv_current_partition_type is zeroed (so GUEST) by default, but I could make that explicit
if you prefer:
+enum hv_partition_type hv_current_partition_type = HV_PARTITION_TYPE_GUEST;

How does that sound? Am I misunderstanding something here?

> <snip>
> 
>> +static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
>> +{
>> +	return hv_result(U64_MAX);
>> +}
> 
> Is there value in perhaps #defining hv_result_<whatever this is> as U64_MAX and returning that for documentation?
> For e.g. assuming this is something like EOPNOTSUPP
> 
> #define HV_RESULT_NOT_SUPP U64_MAX
> 
> static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
> { return hv_result(HV_RESULT_NOT_SUPP); }
> 
The idea here was to copy what hv_do_hypercall does returning U64_MAX in case the hypercall
page is missing, which will hv_result() into an invalid status code. A special value for
that status could make this pattern clearer. I'd want to call out that this isn't a "real"
Hyper-V status code somehow. HV_STATUS's are 16 bits, so it would look more like:

/* "LINUX" because this isn't really a status from the hypervisor.. */
#define HV_STATUS_LINUX_FAIL 0xFFFF
static inline int hv_call_deposit_pages(int node, u64 partition_id, u32 num_pages)
{ return HV_STATUS_LINUX_FAIL; }

Another option: there is another patch coming (which you know of) which maps hypercall
status codes to regular Linux errors like -EOPNOTSUPP. I could simply merge that patch
with this one (or make this a series for v2), and that would result in less churn.
(And leave alone the current use of U64_MAX in hv_do_hypercall, for now).

Nuno

> <snip>
> 
> Thanks,
> Easwar (he/him)