| Message ID | 20240409050302.1523277-24-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user/i386: Properly align signal frame | expand |
On 4/9/24 07:02, Richard Henderson wrote: > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > 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 *)&fxstate->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:
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~
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~
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 *)&fxstate->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. + */ + 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:
Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- linux-user/i386/signal.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)