linux-kernel - Re: [PATCH] x86/hyperv: Restrict get

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] [day] [month] [year] [list]

Message-ID: <20230915065720.GA19997@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Date:   Thu, 14 Sep 2023 23:57:21 -0700
From:   Saurabh Singh Sengar <ssengar@...ux.microsoft.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     ssengar@...rosoft.com, kys@...rosoft.com, haiyangz@...rosoft.com,
        wei.liu@...nel.org, decui@...rosoft.com, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        x86@...nel.org, hpa@...or.com, linux-hyperv@...r.kernel.org,
        linux-kernel@...r.kernel.org, mikelley@...rosoft.com
Subject: Re: [PATCH] x86/hyperv: Restrict get_vtl to only VTL platforms

On Thu, Sep 14, 2023 at 04:49:08PM +0200, Vitaly Kuznetsov wrote:
> Saurabh Sengar <ssengar@...ux.microsoft.com> writes:
> 
> > For non VTL platforms vtl is always 0, and there is no need of
> > get_vtl function. For VTL platforms get_vtl should always succeed
> > and should return the correct VTL.
> >
> > Signed-off-by: Saurabh Sengar <ssengar@...ux.microsoft.com>
> > ---
> >  arch/x86/hyperv/hv_init.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 783ed339f341..e589c240565a 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -416,8 +416,8 @@ static u8 __init get_vtl(void)
> >  	if (hv_result_success(ret)) {
> >  		ret = output->as64.low & HV_X64_VTL_MASK;
> >  	} else {
> > -		pr_err("Failed to get VTL(%lld) and set VTL to zero by default.\n", ret);
> > -		ret = 0;
> > +		pr_err("Failed to get VTL(error: %lld) exiting...\n", ret);
> 
> Nitpick: arch/x86/hyperv/hv_init.c lacks pr_fmt so the message won't get
> prefixed with "Hyper-V". I'm not sure 'VTL' abbreviation has the only,
> Hyper-V specific meaning. I'd suggest we add 
> 
> #define pr_fmt(fmt)  "Hyper-V: " fmt
> 
> to the beginning of the file.

Good suggestion, I can do this as a separate patch.

> 
> > +		BUG();
> >  	}
> >  
> >  	local_irq_restore(flags);
> > @@ -604,8 +604,10 @@ void __init hyperv_init(void)
> >  	hv_query_ext_cap(0);
> >  
> >  	/* Find the VTL */
> > -	if (!ms_hyperv.paravisor_present && hv_isolation_type_snp())
> > +	if (IS_ENABLED(CONFIG_HYPERV_VTL_MODE))
> >  		ms_hyperv.vtl = get_vtl();
> > +	else
> > +		ms_hyperv.vtl = 0;
> 
> Is 'else' branch really needed? 'ms_hyperv' seems to be a statically
> allocated global. But instead of doing this, what about putting the
> whole get_vtl() funtion under '#if (IS_ENABLED(CONFIG_HYPERV_VTL_MODE))', i.e.:
> 
> #if IS_ENABLED(CONFIG_HYPERV_VTL_MODE)
> static u8 __init get_vtl(void)
> {
>         u64 control = HV_HYPERCALL_REP_COMP_1 | HVCALL_GET_VP_REGISTERS;
> ...
> }
> #else
> static inline get_vtl(void) { return 0; }
> #endif
> 
> and then we can always do
> 
>       ms_hyperv.vtl = get_vtl();
> 
> unconditionally?


Fine with me, can update in v2.

- Saurabh

> 
> >  
> >  	return;
> 
> -- 
> Vitaly
>