From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] x86/xsaves: get_xsave_addr needs check the
	xsave header
Date: Fri, 18 Dec 2015 11:13:32 +0000
Message-ID: <5673EA5C.7080305@citrix.com>
References: <1450435810-726-1-git-send-email-huaitong.han@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1450435810-726-1-git-send-email-huaitong.han@intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Huaitong Han <huaitong.han@intel.com>, jbeulich@suse.com, keir@xen.org
Cc: shuai.ruan@linux.intel.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

On 18/12/15 10:50, Huaitong Han wrote:
> The check needs to be against the xsave header in the area, rather than
> Xen's maximum xfeature_mask. A guest might easily have a smaller xcr0
> than the maximum Xen is willing to allow, causing the pointer below to
> be bogus.
>
> Signed-off-by: Huaitong Han <huaitong.han@intel.com>
> ---
>  xen/arch/x86/xstate.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index b65da38..d87ab40 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -146,12 +146,13 @@ static void __init setup_xstate_comp(void)
>      }
>  }
>  
> -static void *get_xsave_addr(void *xsave, unsigned int xfeature_idx)
> +static void *get_xsave_addr(struct xsave_struct *xsave,
> +        unsigned int xfeature_idx)
>  {
> -    if ( !((1ul << xfeature_idx) & xfeature_mask) )
> +    if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
>          return NULL;
>  
> -    return xsave + xstate_comp_offsets[xfeature_idx];
> +    return (void *)xsave + xstate_comp_offsets[xfeature_idx];

This indeed fixes one of the issues.  However, you must also check
xcomb_bv & XSTATE_COMPACTION_ENABLED before using xstate_comp_offsets.

I think you should end up with something like:

if ( xsave->xsave_hdr.xcomb_bv & XSTATE_COMPACTION_ENABLED )
    return (void *)xsave + xstate_comp_offsets[xfeature_idx];
else
    return (void *)xsave + xstate_offsets[xfeature_idx];

which allows get_xsave_addr() to work on both compressed and
uncompressed xstate areas.

~Andrew