Re: [PATCH] x86/fpu: Set the corret permission value for perm.__state_perm

Re: [PATCH] x86/fpu: Set the corret permission value for perm.__state_perm

[Bae, Chang Seok]

On Nov 8, 2021, at 14:28, Zhong, Yang <yang.zhong@intel.com> wrote:
> 
> The perm.__state_perm is default xsave set features, when we request
> AMX permission from application, the requested value(bit18) should be
> replaced with mask value(requested | permitted), which can keep default
> features there.
> 
> Without this change, the below prctl syscall:
> (1). ARCH_GET_XCOMP_PERM, the bitmask=0x202e7
> (2). set ARCH_REQ_XCOMP_PERM with XFEATURE_XTILEDATA
> (3). ARCH_GET_XCOMP_PERM, the bitmask=0x40000, the correct value
>     should be 0x602e7 here.

Oh, bad. 

> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index d28829403ed0..fc1ab0116f4e 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -1626,7 +1626,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested)
> 		return ret;
> 
> 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
> -	WRITE_ONCE(fpu->perm.__state_perm, requested);
> +	WRITE_ONCE(fpu->perm.__state_perm, mask);

I think this has to update the group leader’s like:
    WRITE_ONCE(current->group_leader->thread.fpu.perm.__state_perm, mask);

Thanks,
Chang

[PATCH] x86/fpu: Set the corret permission value for perm.__state_perm

[Yang Zhong]

The perm.__state_perm is default xsave set features, when we request
AMX permission from application, the requested value(bit18) should be
replaced with mask value(requested | permitted), which can keep default
features there.

Without this change, the below prctl syscall:
(1). ARCH_GET_XCOMP_PERM, the bitmask=0x202e7
(2). set ARCH_REQ_XCOMP_PERM with XFEATURE_XTILEDATA
(3). ARCH_GET_XCOMP_PERM, the bitmask=0x40000, the correct value
     should be 0x602e7 here.

Signed-off-by: Yang Zhong <yang.zhong@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index d28829403ed0..fc1ab0116f4e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -1626,7 +1626,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested)
 		return ret;
 
 	/* Pairs with the READ_ONCE() in xstate_get_group_perm() */
-	WRITE_ONCE(fpu->perm.__state_perm, requested);
+	WRITE_ONCE(fpu->perm.__state_perm, mask);
 	/* Protected by sighand lock */
 	fpu->perm.__state_size = ksize;
 	fpu->perm.__user_state_size = usize;
-- 
2.30.2

Re: [PATCH] x86/fpu: Set the corret permission value for perm.__state_perm

[Bae, Chang Seok]

On Nov 8, 2021, at 08:13, Bae, Chang Seok <chang.seok.bae@intel.com> wrote:
> 
> I think this has to update the group leader’s like:
>    WRITE_ONCE(current->group_leader->thread.fpu.perm.__state_perm, mask);

Posted a new patch along with the test update here:
    https://lore.kernel.org/lkml/20211108233501.11516-1-chang.seok.bae@intel.com/

Thanks,
Chang

Re: [PATCH] x86/fpu: Set the corret permission value for perm.__state_perm

[Yang Zhong]

On Tue, Nov 09, 2021 at 12:13:58AM +0800, Bae, Chang Seok wrote:
> On Nov 8, 2021, at 14:28, Zhong, Yang <yang.zhong@intel.com> wrote:
> >
> > The perm.__state_perm is default xsave set features, when we request
> > AMX permission from application, the requested value(bit18) should be
> > replaced with mask value(requested | permitted), which can keep default
> > features there.
> >
> > Without this change, the below prctl syscall:
> > (1). ARCH_GET_XCOMP_PERM, the bitmask=0x202e7
> > (2). set ARCH_REQ_XCOMP_PERM with XFEATURE_XTILEDATA
> > (3). ARCH_GET_XCOMP_PERM, the bitmask=0x40000, the correct value
> >     should be 0x602e7 here.
> 
> Oh, bad.
> 
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index d28829403ed0..fc1ab0116f4e 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -1626,7 +1626,7 @@ static int __xstate_request_perm(u64 permitted, u64 requested)
> >               return ret;
> >
> >       /* Pairs with the READ_ONCE() in xstate_get_group_perm() */
> > -     WRITE_ONCE(fpu->perm.__state_perm, requested);
> > +     WRITE_ONCE(fpu->perm.__state_perm, mask);
> 
> I think this has to update the group leader’s like:
>     WRITE_ONCE(current->group_leader->thread.fpu.perm.__state_perm, mask);
>

  I think this is no need to change with group leader here, the fpu pointer has defined as below: 
  
  struct fpu *fpu = &current->group_leader->thread.fpu;

  Please check Thomas's changes for KVM, the WRITE_ONCE(fpu->perm.__state_perm, mask) is more reasonable
  here.

  https://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git/commit/?h=x86/fpu-kvm&id=62016e4cc41716b33da890ad43145b646f9db51d

  I also verified this changes from host and guest side, all work well.

  Regards,

  Yang
   


> Thanks,
> Chang
> 
> 
>