* Re: [Qemu-devel] [PATCH] target/i386: fix interrupt CPL error when
@ 2017-06-25 11:13 Wu Xiang
2017-06-25 12:10 ` Paolo Bonzini
0 siblings, 1 reply; 2+ messages in thread
From: Wu Xiang @ 2017-06-25 11:13 UTC (permalink / raw)
To: Paolo Bonzini, qemu-devel; +Cc: Eduardo Habkost, RichardHenderson
In-Reply-To: <641e1f35-813a-4244-0194-ffce08130533@redhat.com>
On Fri, Jun 23, 2017 at 01:19:35PM +0200, Paolo Bonzini wrote:
>
>
> On 21/06/2017 16:21, Wu Xiang wrote:
> > In do_interrupt64(), when interrupt stack table(ist) is enabled
> > and the the target code segment is conforming(e2 & DESC_C_MASK), the
> > old implementation always set new CPL to 0, and SS.RPL to 0.
> >
> > This is incorrect for when CPL3 code access a CPL0 conforming code
> > segment, the CPL should remain unchanged. Otherwise higher privileged
> > code can be compromised.
> >
> > The patch fix this for always set dpl = cpl when the target code segment
> > is conforming, and modify the last parameter `flags`, which contains
> > correct new CPL, in cpu_x86_load_seg_cache().
> >
> > Signed-off-by: Wu Xiang <willx8@gmail.com>
> > ---
> > target/i386/seg_helper.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
>
> The patch looks good, but I'm thinking of a cleanup on top that simplifies
> the handling of conforming code segments:
>
> diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> index 9af69c23e0..600a4d7586 100644
> --- a/target/i386/seg_helper.c
> +++ b/target/i386/seg_helper.c
> @@ -692,7 +692,10 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
> if (!(e2 & DESC_P_MASK)) {
> raise_exception_err(env, EXCP0B_NOSEG, selector & 0xfffc);
> }
> - if (!(e2 & DESC_C_MASK) && dpl < cpl) {
> + if (e2 & DESC_C_MASK) {
> + dpl = cpl;
> + }
> + if (dpl < cpl) {
> /* to inner privilege */
> get_ss_esp_from_tss(env, &ss, &esp, dpl, 0);
> if ((ss & 0xfffc) == 0) {
> @@ -719,7 +722,7 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
> new_stack = 1;
> sp_mask = get_sp_mask(ss_e2);
> ssp = get_seg_base(ss_e1, ss_e2);
> - } else if ((e2 & DESC_C_MASK) || dpl == cpl) {
> + } else {
> /* to same privilege */
> if (vm86) {
> raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> @@ -728,13 +731,6 @@ static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
> sp_mask = get_sp_mask(env->segs[R_SS].flags);
> ssp = env->segs[R_SS].base;
> esp = env->regs[R_ESP];
> - dpl = cpl;
> - } else {
> - raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> - new_stack = 0; /* avoid warning */
> - sp_mask = 0; /* avoid warning */
> - ssp = 0; /* avoid warning */
> - esp = 0; /* avoid warning */
> }
>
> shift = type >> 3;
> @@ -919,25 +915,21 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int,
> if (!(e2 & DESC_L_MASK) || (e2 & DESC_B_MASK)) {
> raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> }
> - if ((!(e2 & DESC_C_MASK) && dpl < cpl) || ist != 0) {
> + if (e2 & DESC_C_MASK) {
> + dpl = cpl;
> + }
> + if (dpl < cpl || ist != 0) {
> /* to inner privilege */
> new_stack = 1;
> esp = get_rsp_from_tss(env, ist != 0 ? ist + 3 : dpl);
> ss = 0;
> - } else if ((e2 & DESC_C_MASK) || dpl == cpl) {
> + } else {
> /* to same privilege */
> if (env->eflags & VM_MASK) {
> raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> }
> new_stack = 0;
> esp = env->regs[R_ESP];
> - } else {
> - raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> - new_stack = 0; /* avoid warning */
> - esp = 0; /* avoid warning */
> - }
> - if (e2 & DESC_C_MASK) {
> - dpl = cpl;
> }
> esp &= ~0xfLL; /* align stack */
>
>
> Because dpl == cpl after the new "if", it's now unnecessary to check
> the C bit when testing dpl < cpl. Furthermore, dpl > cpl is checked
> slightly above this code, so the final "else" is unreachable.
>
> What do you think?
>
> Paolo
The patch seems good and clean.
However I am not quite familiar with the code review process. Now that
you have come up with the new patch, do I have to re-send a v2?
Xiang
>
> > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> > index 0374031..9af69c2 100644
> > --- a/target/i386/seg_helper.c
> > +++ b/target/i386/seg_helper.c
> > @@ -931,12 +931,14 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int,
> > }
> > new_stack = 0;
> > esp = env->regs[R_ESP];
> > - dpl = cpl;
> > } else {
> > raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> > new_stack = 0; /* avoid warning */
> > esp = 0; /* avoid warning */
> > }
> > + if (e2 & DESC_C_MASK) {
> > + dpl = cpl;
> > + }
> > esp &= ~0xfLL; /* align stack */
> >
> > PUSHQ(esp, env->segs[R_SS].selector);
> > @@ -956,7 +958,7 @@ static void do_interrupt64(CPUX86State *env, int intno, int is_int,
> >
> > if (new_stack) {
> > ss = 0 | dpl;
> > - cpu_x86_load_seg_cache(env, R_SS, ss, 0, 0, 0);
> > + cpu_x86_load_seg_cache(env, R_SS, ss, 0, 0, dpl << DESC_DPL_SHIFT);
> > }
> > env->regs[R_ESP] = esp;
> >
> >
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Qemu-devel] [PATCH] target/i386: fix interrupt CPL error when
2017-06-25 11:13 [Qemu-devel] [PATCH] target/i386: fix interrupt CPL error when Wu Xiang
@ 2017-06-25 12:10 ` Paolo Bonzini
0 siblings, 0 replies; 2+ messages in thread
From: Paolo Bonzini @ 2017-06-25 12:10 UTC (permalink / raw)
To: Wu Xiang; +Cc: qemu-devel, Eduardo Habkost, RichardHenderson
----- Original Message -----
> From: "Wu Xiang" <willx8@gmail.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Cc: "Eduardo Habkost" <ehabkost@redhat.com>, "RichardHenderson" <rth@twiddle.net>
> Sent: Sunday, June 25, 2017 1:13:24 PM
> Subject: Re: [Qemu-devel][PATCH] target/i386: fix interrupt CPL error when
>
> In-Reply-To: <641e1f35-813a-4244-0194-ffce08130533@redhat.com>
>
> On Fri, Jun 23, 2017 at 01:19:35PM +0200, Paolo Bonzini wrote:
> >
> >
> > On 21/06/2017 16:21, Wu Xiang wrote:
> > > In do_interrupt64(), when interrupt stack table(ist) is enabled
> > > and the the target code segment is conforming(e2 & DESC_C_MASK), the
> > > old implementation always set new CPL to 0, and SS.RPL to 0.
> > >
> > > This is incorrect for when CPL3 code access a CPL0 conforming code
> > > segment, the CPL should remain unchanged. Otherwise higher privileged
> > > code can be compromised.
> > >
> > > The patch fix this for always set dpl = cpl when the target code segment
> > > is conforming, and modify the last parameter `flags`, which contains
> > > correct new CPL, in cpu_x86_load_seg_cache().
> > >
> > > Signed-off-by: Wu Xiang <willx8@gmail.com>
> > > ---
> > > target/i386/seg_helper.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > The patch looks good, but I'm thinking of a cleanup on top that simplifies
> > the handling of conforming code segments:
> >
> > diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
> > index 9af69c23e0..600a4d7586 100644
> > --- a/target/i386/seg_helper.c
> > +++ b/target/i386/seg_helper.c
> > @@ -692,7 +692,10 @@ static void do_interrupt_protected(CPUX86State *env,
> > int intno, int is_int,
> > if (!(e2 & DESC_P_MASK)) {
> > raise_exception_err(env, EXCP0B_NOSEG, selector & 0xfffc);
> > }
> > - if (!(e2 & DESC_C_MASK) && dpl < cpl) {
> > + if (e2 & DESC_C_MASK) {
> > + dpl = cpl;
> > + }
> > + if (dpl < cpl) {
> > /* to inner privilege */
> > get_ss_esp_from_tss(env, &ss, &esp, dpl, 0);
> > if ((ss & 0xfffc) == 0) {
> > @@ -719,7 +722,7 @@ static void do_interrupt_protected(CPUX86State *env,
> > int intno, int is_int,
> > new_stack = 1;
> > sp_mask = get_sp_mask(ss_e2);
> > ssp = get_seg_base(ss_e1, ss_e2);
> > - } else if ((e2 & DESC_C_MASK) || dpl == cpl) {
> > + } else {
> > /* to same privilege */
> > if (vm86) {
> > raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> > @@ -728,13 +731,6 @@ static void do_interrupt_protected(CPUX86State *env,
> > int intno, int is_int,
> > sp_mask = get_sp_mask(env->segs[R_SS].flags);
> > ssp = env->segs[R_SS].base;
> > esp = env->regs[R_ESP];
> > - dpl = cpl;
> > - } else {
> > - raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> > - new_stack = 0; /* avoid warning */
> > - sp_mask = 0; /* avoid warning */
> > - ssp = 0; /* avoid warning */
> > - esp = 0; /* avoid warning */
> > }
> >
> > shift = type >> 3;
> > @@ -919,25 +915,21 @@ static void do_interrupt64(CPUX86State *env, int
> > intno, int is_int,
> > if (!(e2 & DESC_L_MASK) || (e2 & DESC_B_MASK)) {
> > raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> > }
> > - if ((!(e2 & DESC_C_MASK) && dpl < cpl) || ist != 0) {
> > + if (e2 & DESC_C_MASK) {
> > + dpl = cpl;
> > + }
> > + if (dpl < cpl || ist != 0) {
> > /* to inner privilege */
> > new_stack = 1;
> > esp = get_rsp_from_tss(env, ist != 0 ? ist + 3 : dpl);
> > ss = 0;
> > - } else if ((e2 & DESC_C_MASK) || dpl == cpl) {
> > + } else {
> > /* to same privilege */
> > if (env->eflags & VM_MASK) {
> > raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> > }
> > new_stack = 0;
> > esp = env->regs[R_ESP];
> > - } else {
> > - raise_exception_err(env, EXCP0D_GPF, selector & 0xfffc);
> > - new_stack = 0; /* avoid warning */
> > - esp = 0; /* avoid warning */
> > - }
> > - if (e2 & DESC_C_MASK) {
> > - dpl = cpl;
> > }
> > esp &= ~0xfLL; /* align stack */
> >
> >
> > Because dpl == cpl after the new "if", it's now unnecessary to check
> > the C bit when testing dpl < cpl. Furthermore, dpl > cpl is checked
> > slightly above this code, so the final "else" is unreachable.
> >
> > What do you think?
> >
> > Paolo
>
> The patch seems good and clean.
>
> However I am not quite familiar with the code review process. Now that
> you have come up with the new patch, do I have to re-send a v2?
No, your patch is okay. Mine comes after yours, I'll send it out
tomorrow.
Paolo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-06-25 12:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-06-25 11:13 [Qemu-devel] [PATCH] target/i386: fix interrupt CPL error when Wu Xiang
2017-06-25 12:10 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).