linux-kernel - Re: [PATCH v3 11/21] x86/fpu/xstate: Update xstate buffer address finder to support dynamic xstate

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]

Date:   Fri, 19 Feb 2021 16:00:50 +0100
From:   Borislav Petkov <bp@...e.de>
To:     "Chang S. Bae" <chang.seok.bae@...el.com>
Cc:     luto@...nel.org, tglx@...utronix.de, mingo@...nel.org,
        x86@...nel.org, len.brown@...el.com, dave.hansen@...el.com,
        jing2.liu@...el.com, ravi.v.shankar@...el.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 11/21] x86/fpu/xstate: Update xstate buffer address
 finder to support dynamic xstate

On Wed, Dec 23, 2020 at 07:57:07AM -0800, Chang S. Bae wrote:
> __raw_xsave_addr() returns the requested component's pointer in an xstate
> buffer, by simply looking up the offset table. The offset used to be fixed,
> but, with dynamic user states, it becomes variable.
> 
> get_xstate_size() has a routine to find an offset at runtime. Refactor to
> use it for the address finder.
> 
> No functional change until the kernel enables dynamic user states.
> 
> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Reviewed-by: Len Brown <len.brown@...el.com>
> Cc: x86@...nel.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  arch/x86/kernel/fpu/xstate.c | 82 +++++++++++++++++++++++-------------
>  1 file changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 8dfbc7d1702a..6b863b2ca405 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -133,15 +133,50 @@ static bool xfeature_is_supervisor(int xfeature_nr)
>  	return ecx & 1;
>  }
>  
> +/*
> + * Available once those arrays for the offset, size, and alignment info are set up,
> + * by setup_xstate_features().
> + */

That's kinda clear, right? Apparently, we do cache FPU attributes in
xstate.c so what is that comment actually trying to tell us? Or do you
want to add some sort of an assertion to this function in case it gets
called before setup_xstate_features()?

I think you should simply add kernel-doc style comment explaining what
the inputs are and what the function gives, which would be a lot more
useful.

> +static unsigned int __get_xstate_comp_offset(u64 mask, int feature_nr)
> +{
> +	u64 xmask = BIT_ULL(feature_nr + 1) - 1;
> +	unsigned int next_offset, offset = 0;
> +	int i;
> +
> +	if ((mask & xmask) == (xfeatures_mask_all & xmask))
> +		return xstate_comp_offsets[feature_nr];
> +
> +	/*
> +	 * Calculate the size by summing up each state together, since no known
> +	 * offset found with the xstate buffer format out of the given mask.
> +	 */
> +
> +	next_offset = FXSAVE_SIZE + XSAVE_HDR_SIZE;
> +
> +	for (i = FIRST_EXTENDED_XFEATURE; i <= feature_nr; i++) {
> +		if (!(mask & BIT_ULL(i)))
> +			continue;
> +
> +		offset = xstate_aligns[i] ? ALIGN(next_offset, 64) : next_offset;
> +		next_offset += xstate_sizes[i];
> +	}
> +
> +	return offset;
> +}
> +
> +static unsigned int get_xstate_comp_offset(struct fpu *fpu, int feature_nr)
> +{
> +	return __get_xstate_comp_offset(fpu->state_mask, feature_nr);
> +}

Just get rid of the __ variant and have a single function with the
following signature:

	static unsigned int get_xstate_comp_offset(u64 mask, int feature_nr)


Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer, HRB 36809, AG Nürnberg