linux-kernel - Re: [PATCH v3 15/15] Drivers: hv: Add modules to expose /dev/mshv to VMMs running on Hyper-V

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e235025e-abfa-4b31-8b83-416ec8ec4f72@linux.microsoft.com>
Date:   Mon, 25 Sep 2023 17:07:24 -0700
From:   Nuno Das Neves <nunodasneves@...ux.microsoft.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org,
        x86@...nel.org, linux-arm-kernel@...ts.infradead.org,
        linux-arch@...r.kernel.org, patches@...ts.linux.dev,
        mikelley@...rosoft.com, kys@...rosoft.com, wei.liu@...nel.org,
        haiyangz@...rosoft.com, decui@...rosoft.com,
        apais@...ux.microsoft.com, Tianyu.Lan@...rosoft.com,
        ssengar@...ux.microsoft.com, mukeshrathor@...rosoft.com,
        stanislav.kinsburskiy@...il.com, jinankjain@...ux.microsoft.com,
        vkuznets@...hat.com, tglx@...utronix.de, mingo@...hat.com,
        bp@...en8.de, dave.hansen@...ux.intel.com, hpa@...or.com,
        will@...nel.org, catalin.marinas@....com
Subject: Re: [PATCH v3 15/15] Drivers: hv: Add modules to expose /dev/mshv to
 VMMs running on Hyper-V

Resend in plain text instead of HTML - oops!

On 9/23/2023 12:58 AM, Greg KH wrote:
> On Fri, Sep 22, 2023 at 11:38:35AM -0700, Nuno Das Neves wrote:
>> +static int mshv_vtl_get_vsm_regs(void)
>> +{
>> +	struct hv_register_assoc registers[2];
>> +	union hv_input_vtl input_vtl;
>> +	int ret, count = 2;
>> +
>> +	input_vtl.as_uint8 = 0;
>> +	registers[0].name = HV_REGISTER_VSM_CODE_PAGE_OFFSETS;
>> +	registers[1].name = HV_REGISTER_VSM_CAPABILITIES;
>> +
>> +	ret = hv_call_get_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> +				       count, input_vtl, registers);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mshv_vsm_page_offsets.as_uint64 = registers[0].value.reg64;
>> +	mshv_vsm_capabilities.as_uint64 = registers[1].value.reg64;
>> +
>> +	pr_debug("%s: VSM code page offsets: %#016llx\n", __func__,
>> +		 mshv_vsm_page_offsets.as_uint64);
>> +	pr_info("%s: VSM capabilities: %#016llx\n", __func__,
>> +		mshv_vsm_capabilities.as_uint64);
> 
> When drivers are working properly, they are quiet.  This is very noisy
> and probably is leaking memory addresses to userspace?
> 

I will remove these, thanks.

> Also, there is NEVER a need for __func__ in a pr_debug() line, it has
> that for you automatically.
> 

Thank you, I didn't know this.

> Also, drivers should never call pr_*() calls, always use the proper
> dev_*() calls instead.
> 

We only use struct device in one place in this driver, I think that is 
the only place it makes sense to use dev_*() over pr_*() calls.
> 
> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int mshv_vtl_configure_vsm_partition(void)
>> +{
>> +	union hv_register_vsm_partition_config config;
>> +	struct hv_register_assoc reg_assoc;
>> +	union hv_input_vtl input_vtl;
>> +
>> +	config.as_u64 = 0;
>> +	config.default_vtl_protection_mask = HV_MAP_GPA_PERMISSIONS_MASK;
>> +	config.enable_vtl_protection = 1;
>> +	config.zero_memory_on_reset = 1;
>> +	config.intercept_vp_startup = 1;
>> +	config.intercept_cpuid_unimplemented = 1;
>> +
>> +	if (mshv_vsm_capabilities.intercept_page_available) {
>> +		pr_debug("%s: using intercept page", __func__);
> 
> Again, __func__ is not needed, you are providing it twice here for no
> real reason except to waste storage space :)
> 

Thanks, I will review all the uses of pr_debug().

>> +		config.intercept_page = 1;
>> +	}
>> +
>> +	reg_assoc.name = HV_REGISTER_VSM_PARTITION_CONFIG;
>> +	reg_assoc.value.reg64 = config.as_u64;
>> +	input_vtl.as_uint8 = 0;
>> +
>> +	return hv_call_set_vp_registers(HV_VP_INDEX_SELF, HV_PARTITION_ID_SELF,
>> +				       1, input_vtl, &reg_assoc);
> 
> 
> None of this needs to be unwound if initialization fails later on?
> 

I think unwinding this is not needed, not 100% sure.
Saurabh, can you comment?

Thanks,
Nuno

> thanks,
> 
> greg k-h