arch/x86: Fix calculation of xstate_size

Submitted by Cyrill Gorcunov on May 18, 2020, 8:19 a.m.

Details

Message ID 20200518081923.78314-1-gorcunov@gmail.com
State New
Series "arch/x86: Fix calculation of xstate_size"
Headers show

Commit Message

Cyrill Gorcunov May 18, 2020, 8:19 a.m.
The layout of xsave frame in a standart format is predefined by the hardware.
Lets make sure we're increasing in frame offsets and use latest offset where
appropriate.

https://github.com/checkpoint-restore/criu/issues/1042

Reported-by: Ashutosh Mehra <mehra.ashutosh@ibm.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 criu/arch/x86/crtools.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
index 9c8beeedddf0..bc80225358ff 100644
--- a/criu/arch/x86/crtools.c
+++ b/criu/arch/x86/crtools.c
@@ -437,6 +437,7 @@  int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
 			void *from = xsave->member;					\
 			size_t size = pb_repeated_size(xsave, member);			\
 			size_t xsize = (size_t)compel_fpu_feature_size(feature);	\
+			size_t xstate_size_next = off + xsize;				\
 			if (xsize != size) {						\
 				if (size) {						\
 					pr_err("%s reported %zu bytes (expecting %zu)\n",\
@@ -448,7 +449,8 @@  int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
 				}							\
 			}								\
 			xstate_bv |= (1UL << feature);					\
-			xstate_size += xsize;						\
+			BUG_ON(xstate_size > xstate_size_next);				\
+			xstate_size = xstate_size_next;					\
 			memcpy(to, from, size);						\
 		}									\
 	} while (0)
@@ -485,6 +487,11 @@  int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
 			UserX86XsaveEntry *xsave = core->thread_info->fpregs->xsave;
 			uint8_t *extended_state_area = (void *)x;
 
+			/*
+			 * Note the order does matter here and bound
+			 * to the increasing offsets of XFEATURE_x
+			 * inside memory layout (xstate_size calculation).
+			 */
 			assign_xsave(XFEATURE_YMM,	xsave, ymmh_space,	extended_state_area);
 			assign_xsave(XFEATURE_BNDREGS,	xsave, bndreg_state,	extended_state_area);
 			assign_xsave(XFEATURE_BNDCSR,	xsave, bndcsr_state,	extended_state_area);

Comments

Andrei Vagin May 31, 2020, 9:55 a.m.
Hi Cyrill and Dmitry,

Dima, could you review this patch?
Cyrill, could you create a pull-request on github?

Thanks,
Andrei

On Mon, May 18, 2020 at 1:19 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> The layout of xsave frame in a standart format is predefined by the hardware.
> Lets make sure we're increasing in frame offsets and use latest offset where
> appropriate.
>
> https://github.com/checkpoint-restore/criu/issues/1042
>
> Reported-by: Ashutosh Mehra <mehra.ashutosh@ibm.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  criu/arch/x86/crtools.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
> index 9c8beeedddf0..bc80225358ff 100644
> --- a/criu/arch/x86/crtools.c
> +++ b/criu/arch/x86/crtools.c
> @@ -437,6 +437,7 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>                         void *from = xsave->member;                                     \
>                         size_t size = pb_repeated_size(xsave, member);                  \
>                         size_t xsize = (size_t)compel_fpu_feature_size(feature);        \
> +                       size_t xstate_size_next = off + xsize;                          \
>                         if (xsize != size) {                                            \
>                                 if (size) {                                             \
>                                         pr_err("%s reported %zu bytes (expecting %zu)\n",\
> @@ -448,7 +449,8 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>                                 }                                                       \
>                         }                                                               \
>                         xstate_bv |= (1UL << feature);                                  \
> -                       xstate_size += xsize;                                           \
> +                       BUG_ON(xstate_size > xstate_size_next);                         \
> +                       xstate_size = xstate_size_next;                                 \
>                         memcpy(to, from, size);                                         \
>                 }                                                                       \
>         } while (0)
> @@ -485,6 +487,11 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>                         UserX86XsaveEntry *xsave = core->thread_info->fpregs->xsave;
>                         uint8_t *extended_state_area = (void *)x;
>
> +                       /*
> +                        * Note the order does matter here and bound
> +                        * to the increasing offsets of XFEATURE_x
> +                        * inside memory layout (xstate_size calculation).
> +                        */
>                         assign_xsave(XFEATURE_YMM,      xsave, ymmh_space,      extended_state_area);
>                         assign_xsave(XFEATURE_BNDREGS,  xsave, bndreg_state,    extended_state_area);
>                         assign_xsave(XFEATURE_BNDCSR,   xsave, bndcsr_state,    extended_state_area);
> --
> 2.26.2
>
Cyrill Gorcunov May 31, 2020, 6:54 p.m.
On Sun, May 31, 2020 at 02:55:22AM -0700, Andrei Vagin wrote:
> Hi Cyrill and Dmitry,
> 
> Dima, could you review this patch?
> Cyrill, could you create a pull-request on github?
>

Andrew, I wonder -- now all patch should go via PR?
Andrei Vagin May 31, 2020, 7:57 p.m.
On Sun, May 31, 2020 at 11:54 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>
> On Sun, May 31, 2020 at 02:55:22AM -0700, Andrei Vagin wrote:
> > Hi Cyrill and Dmitry,
> >
> > Dima, could you review this patch?
> > Cyrill, could you create a pull-request on github?
> >
>
> Andrew, I wonder -- now all patch should go via PR?

yes. This is the preferred way.
Dmitry Safonov June 1, 2020, 9:28 a.m.
On 5/31/20 10:55 AM, Andrei Vagin wrote:
> Hi Cyrill and Dmitry,
> 
> Dima, could you review this patch?
> Cyrill, could you create a pull-request on github?

The patch looks reasonable to me.
I don't like introducing new BUG_ON's, but that could be addressed on
the top.

> 
> Thanks,
> Andrei
> 
> On Mon, May 18, 2020 at 1:19 AM Cyrill Gorcunov <gorcunov@gmail.com> wrote:
>>
>> The layout of xsave frame in a standart format is predefined by the hardware.
>> Lets make sure we're increasing in frame offsets and use latest offset where
>> appropriate.
>>
>> https://github.com/checkpoint-restore/criu/issues/1042
>>
>> Reported-by: Ashutosh Mehra <mehra.ashutosh@ibm.com>
>> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

Reviewed-by: Dmitry Safonov <0x7f454c46@gmail.com>

>> ---
>>  criu/arch/x86/crtools.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
>> index 9c8beeedddf0..bc80225358ff 100644
>> --- a/criu/arch/x86/crtools.c
>> +++ b/criu/arch/x86/crtools.c
>> @@ -437,6 +437,7 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>>                         void *from = xsave->member;                                     \
>>                         size_t size = pb_repeated_size(xsave, member);                  \
>>                         size_t xsize = (size_t)compel_fpu_feature_size(feature);        \
>> +                       size_t xstate_size_next = off + xsize;                          \
>>                         if (xsize != size) {                                            \
>>                                 if (size) {                                             \
>>                                         pr_err("%s reported %zu bytes (expecting %zu)\n",\
>> @@ -448,7 +449,8 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>>                                 }                                                       \
>>                         }                                                               \
>>                         xstate_bv |= (1UL << feature);                                  \
>> -                       xstate_size += xsize;                                           \
>> +                       BUG_ON(xstate_size > xstate_size_next);                         \
>> +                       xstate_size = xstate_size_next;                                 \
>>                         memcpy(to, from, size);                                         \
>>                 }                                                                       \
>>         } while (0)
>> @@ -485,6 +487,11 @@ int restore_fpu(struct rt_sigframe *sigframe, CoreEntry *core)
>>                         UserX86XsaveEntry *xsave = core->thread_info->fpregs->xsave;
>>                         uint8_t *extended_state_area = (void *)x;
>>
>> +                       /*
>> +                        * Note the order does matter here and bound
>> +                        * to the increasing offsets of XFEATURE_x
>> +                        * inside memory layout (xstate_size calculation).
>> +                        */
>>                         assign_xsave(XFEATURE_YMM,      xsave, ymmh_space,      extended_state_area);
>>                         assign_xsave(XFEATURE_BNDREGS,  xsave, bndreg_state,    extended_state_area);
>>                         assign_xsave(XFEATURE_BNDCSR,   xsave, bndcsr_state,    extended_state_area);
>> --
>> 2.26.2
>>

Thanks,
          Dima
Cyrill Gorcunov June 1, 2020, 9:42 a.m.
On Mon, Jun 01, 2020 at 10:28:19AM +0100, Dmitry Safonov wrote:
> On 5/31/20 10:55 AM, Andrei Vagin wrote:
> > Hi Cyrill and Dmitry,
> > 
> > Dima, could you review this patch?
> > Cyrill, could you create a pull-request on github?
> 
> The patch looks reasonable to me.
> I don't like introducing new BUG_ON's, but that could be addressed on
> the top.

BUG_ON addresses a screwed hardware, so if it triggers it is indeed a
serious issue and must never happen in real life.