Re: [PATCH v2 23/28] target/i386: Honor xfeatures in xrstor_sigcontext
On 4/9/24 08:09, Richard Henderson wrote:
On 4/8/24 21:44, Paolo Bonzini wrote:
+ /*
+ * Restore the features indicated in the frame, masked by
+ * those currently enabled. Re-check the frame size.
+ * ??? It is not clear where the kernel does this, but it
+ * is not in check_xstate_in_sigframe, and so (probably)
+ * does not fall back to fxrstor.
+ */
I think you're referring to this in __fpu_restore_sig?
if (use_xsave()) {
/*
* Remove all UABI feature bits not set in user_xfeatures
* from the memory xstate header which makes the full
* restore below bring them into init state. This works for
* fx_only mode as well because that has only FP and SSE
* set in user_xfeatures.
*
* Preserve supervisor states!
*/
u64 mask = user_xfeatures | xfeatures_mask_supervisor();
fpregs->xsave.header.xfeatures &= mask;
success = !os_xrstor_safe(fpu->fpstate,
fpu_kernel_cfg.max_features);
It is not masking against the user process's xcr0, but qemu-user's xcr0
is effectively user_xfeatures (it's computed in x86_cpu_reset_hold() and
will never change afterwards since XSETBV is privileged).
No, I'm talking about verifying that the xstate_size is large enough.
In check_xstate_in_sigframe,
if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
fx_sw->xstate_size < min_xstate_size ||
Check for the trivially too small case (fxregs + header).
fx_sw->xstate_size > current->thread.fpu.fpstate->user_size ||
Check for the trivially too large case (presumably this is to catch stupidly large values,
assuming garbage).
fx_sw->xstate_size > fx_sw->extended_size)
Check for trivial mismatch between fields.
goto setfx;
But there's a middle case: if xfeatures > 3, then xstate_size must be >
min_xstate_size.
I know that the kernel will handle any #GP in xrstor_from_user_sigframe, but there doesn't
seem to be a real check for reading garbage beyond the given size.
Oh, I meant to mention, following the
__fpu_restore_sig:
user_xfeatures = fx_sw_user.xfeatures;
...
if (!ia32_fxstate)
restore_fpregs_from_user(..., user_xfeatures, ...)
restore_fpregs_from_user(..., xrestore, ...)
xrestore &= fpu->fpstate->user_xfeatures;
__restore_fpregs_from_user(..., xrestore, ...)
path.
r~
Re: [PATCH v2 23/28] target/i386: Honor xfeatures in xrstor_sigcontext
On 4/8/24 21:44, Paolo Bonzini wrote:
+ /*
+ * Restore the features indicated in the frame, masked by
+ * those currently enabled. Re-check the frame size.
+ * ??? It is not clear where the kernel does this, but it
+ * is not in check_xstate_in_sigframe, and so (probably)
+ * does not fall back to fxrstor.
+ */
I think you're referring to this in __fpu_restore_sig?
if (use_xsave()) {
/*
* Remove all UABI feature bits not set in user_xfeatures
* from the memory xstate header which makes the full
* restore below bring them into init state. This works for
* fx_only mode as well because that has only FP and SSE
* set in user_xfeatures.
*
* Preserve supervisor states!
*/
u64 mask = user_xfeatures | xfeatures_mask_supervisor();
fpregs->xsave.header.xfeatures &= mask;
success = !os_xrstor_safe(fpu->fpstate,
fpu_kernel_cfg.max_features);
It is not masking against the user process's xcr0, but qemu-user's xcr0
is effectively user_xfeatures (it's computed in x86_cpu_reset_hold() and
will never change afterwards since XSETBV is privileged).
No, I'm talking about verifying that the xstate_size is large enough.
In check_xstate_in_sigframe,
if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
fx_sw->xstate_size < min_xstate_size ||
Check for the trivially too small case (fxregs + header).
fx_sw->xstate_size > current->thread.fpu.fpstate->user_size ||
Check for the trivially too large case (presumably this is to catch stupidly large values,
assuming garbage).
fx_sw->xstate_size > fx_sw->extended_size)
Check for trivial mismatch between fields.
goto setfx;
But there's a middle case: if xfeatures > 3, then xstate_size must be >
min_xstate_size.
I know that the kernel will handle any #GP in xrstor_from_user_sigframe, but there doesn't
seem to be a real check for reading garbage beyond the given size.
r~
Re: [PATCH v2 23/28] target/i386: Honor xfeatures in xrstor_sigcontext
On 4/9/24 07:02, Richard Henderson wrote:
Signed-off-by: Richard Henderson
---
linux-user/i386/signal.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
index d015fe520a..fd09c973d4 100644
--- a/linux-user/i386/signal.c
+++ b/linux-user/i386/signal.c
@@ -612,6 +612,7 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind
fpkind,
struct target_fpx_sw_bytes *sw = (void *)>sw_reserved;
uint32_t magic1, magic2;
uint32_t extended_size, xstate_size, min_size, max_size;
+uint64_t xfeatures;
switch (fpkind) {
case FPSTATE_XSAVE:
@@ -628,10 +629,25 @@ static bool xrstor_sigcontext(CPUX86State *env,
FPStateKind fpkind,
xstate_size > extended_size) {
break;
}
+
+/*
+ * Restore the features indicated in the frame, masked by
+ * those currently enabled. Re-check the frame size.
+ * ??? It is not clear where the kernel does this, but it
+ * is not in check_xstate_in_sigframe, and so (probably)
+ * does not fall back to fxrstor.
+ */
I think you're referring to this in __fpu_restore_sig?
if (use_xsave()) {
/*
* Remove all UABI feature bits not set in user_xfeatures
* from the memory xstate header which makes the full
* restore below bring them into init state. This works for
* fx_only mode as well because that has only FP and SSE
* set in user_xfeatures.
*
* Preserve supervisor states!
*/
u64 mask = user_xfeatures | xfeatures_mask_supervisor();
fpregs->xsave.header.xfeatures &= mask;
success = !os_xrstor_safe(fpu->fpstate,
fpu_kernel_cfg.max_features);
It is not masking against the user process's xcr0, but qemu-user's xcr0
is effectively user_xfeatures (it's computed in x86_cpu_reset_hold() and
will never change afterwards since XSETBV is privileged).
Paolo
+xfeatures = tswap64(sw->xfeatures) & env->xcr0;
+min_size = xsave_area_size(xfeatures, false);
+if (xstate_size < min_size) {
+return false;
+}
+
if (!access_ok(env_cpu(env), VERIFY_READ, fxstate_addr,
xstate_size + TARGET_FP_XSTATE_MAGIC2_SIZE)) {
return false;
}
+
/*
* Check for the presence of second magic word at the end of memory
* layout. This detects the case where the user just copied the legacy
@@ -644,7 +660,8 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind
fpkind,
if (magic2 != FP_XSTATE_MAGIC2) {
break;
}
-cpu_x86_xrstor(env, fxstate_addr, -1);
+
+cpu_x86_xrstor(env, fxstate_addr, xfeatures);
return true;
default:
