Re: [ros-dev] [ros-diffs] [aandrejevic] 65441: [FAST486] Attempt to write the value to the stack before subtracting from the actual ESP register in Fast486StackPush.

[Timo Kreuzer]

Are you sure this is the correct order of execution? The instruction is
documented as first lowering the stack pointer, then writing to the new
location.
If a stack exception occurs, you might have esp off by 4.
Am 21.11.2014 10:46, schrieb aandrejevic(a)svn.reactos.org:
>   Author: aandrejevic
>  Date: Fri Nov 21 09:46:57 2014
>  New Revision: 65441
>  URL: http://svn.reactos.org/svn/reactos?rev=65441&view=rev
>  Log:
>  [FAST486]
>  Attempt to write the value to the stack before subtracting from the actual ESP register
>  in Fast486StackPush.
>  Modified:
>       trunk/reactos/lib/fast486/common.inl
>  Modified: trunk/reactos/lib/fast486/common.inl
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/lib/fast486/common.inl?rev…
>  ==============================================================================
>  --- trunk/reactos/lib/fast486/common.inl       [iso-8859-1] (original)
>  +++ trunk/reactos/lib/fast486/common.inl       [iso-8859-1] Fri Nov 21 09:46:57 2014
>  @@ -317,15 +317,19 @@
>                return FALSE;
>            }
>  +        /* Store the value in SS:[ESP - 4] */
>  +        if (!Fast486WriteMemory(State,
>  +                                FAST486_REG_SS,
>  +                                State->GeneralRegs[FAST486_REG_ESP].Long -
> sizeof(ULONG),
>  +                                &Value,
>  +                                sizeof(ULONG)))
>  +        {
>  +            /* Exception occurred */
>  +            return FALSE;
>  +        }
>  +
>            /* Subtract ESP by 4 */
>            State->GeneralRegs[FAST486_REG_ESP].Long -= sizeof(ULONG);
>  -
>  -        /* Store the value in SS:ESP */
>  -        return Fast486WriteMemory(State,
>  -                                  FAST486_REG_SS,
>  -                                  State->GeneralRegs[FAST486_REG_ESP].Long,
>  -                                  &Value,
>  -                                  sizeof(ULONG));
>        }
>        else
>        {
>  @@ -339,16 +343,22 @@
>                return FALSE;
>            }
>  +        /* Store the value in SS:[SP - 2] */
>  +        if (!Fast486WriteMemory(State,
>  +                                FAST486_REG_SS,
>  +                                LOWORD(State->GeneralRegs[FAST486_REG_ESP].LowWord -
> sizeof(USHORT)),
>  +                                &ShortValue,
>  +                                sizeof(USHORT)))
>  +        {
>  +            /* Exception occurred */
>  +            return FALSE;
>  +        }
>  +
>            /* Subtract SP by 2 */
>            State->GeneralRegs[FAST486_REG_ESP].LowWord -= sizeof(USHORT);
>  -
>  -        /* Store the value in SS:SP */
>  -        return Fast486WriteMemory(State,
>  -                                  FAST486_REG_SS,
>  -                                  State->GeneralRegs[FAST486_REG_ESP].LowWord,
>  -                                  &ShortValue,
>  -                                  sizeof(USHORT));
>  -    }
>  +    }
>  +
>  +    return TRUE;
>    }
>    FORCEINLINE
>

[Alexander Andrejevic]

Yes, I'm sure. Consider the case when a page fault occurs. The page fault handler can
choose to map the page the instruction attempted to write to and then retry the
instruction.
Now, if ESP was already lowered, it will be lowered again, which is wrong.
In fact, this patch fixes the linux kernel fork() copy on write problem (when running
linux
under NTVDM).
Regards,
Alex
On Fri, Nov 21, 2014 at 11:55:18PM +0100, Timo Kreuzer wrote:
>   Are you sure this is the correct order of execution?
> The instruction is
>  documented as first lowering the stack pointer, then writing to the new
>  location.
>  If a stack exception occurs, you might have esp off by 4.
>  Am 21.11.2014 10:46, schrieb aandrejevic(a)svn.reactos.org:
> >  Author: aandrejevic
> > Date: Fri Nov 21 09:46:57 2014
> > New Revision: 65441
> > URL: http://svn.reactos.org/svn/reactos?rev=65441&view=rev
> > Log:
> > [FAST486]
> > Attempt to write the value to the stack before subtracting from the actual ESP register
> > in Fast486StackPush.
> > Modified:
> >      trunk/reactos/lib/fast486/common.inl
> > Modified: trunk/reactos/lib/fast486/common.inl
> > URL:
> > http://svn.reactos.org/svn/reactos/trunk/reactos/lib/fast486/common.inl?rev…
> > ==============================================================================
> > --- trunk/reactos/lib/fast486/common.inl        [iso-8859-1] (original)
> > +++ trunk/reactos/lib/fast486/common.inl        [iso-8859-1] Fri Nov 21 09:46:57 2014
> > @@ -317,15 +317,19 @@
> >               return FALSE;
> >           }
> > +        /* Store the value in SS:[ESP - 4] */
> > +        if (!Fast486WriteMemory(State,
> > +                                FAST486_REG_SS,
> > +                                State->GeneralRegs[FAST486_REG_ESP].Long -
> > sizeof(ULONG),
> > +                                &Value,
> > +                                sizeof(ULONG)))
> > +        {
> > +            /* Exception occurred */
> > +            return FALSE;
> > +        }
> > +
> >           /* Subtract ESP by 4 */
> >           State->GeneralRegs[FAST486_REG_ESP].Long -= sizeof(ULONG);
> > -
> > -        /* Store the value in SS:ESP */
> > -        return Fast486WriteMemory(State,
> > -                                  FAST486_REG_SS,
> > -                                  State->GeneralRegs[FAST486_REG_ESP].Long,
> > -                                  &Value,
> > -                                  sizeof(ULONG));
> >       }
> >       else
> >       {
> > @@ -339,16 +343,22 @@
> >               return FALSE;
> >           }
> > +        /* Store the value in SS:[SP - 2] */
> > +        if (!Fast486WriteMemory(State,
> > +                                FAST486_REG_SS,
> > +                                LOWORD(State->GeneralRegs[FAST486_REG_ESP].LowWord -
> > sizeof(USHORT)),
> > +                                &ShortValue,
> > +                                sizeof(USHORT)))
> > +        {
> > +            /* Exception occurred */
> > +            return FALSE;
> > +        }
> > +
> >           /* Subtract SP by 2 */
> >           State->GeneralRegs[FAST486_REG_ESP].LowWord -= sizeof(USHORT);
> > -
> > -        /* Store the value in SS:SP */
> > -        return Fast486WriteMemory(State,
> > -                                  FAST486_REG_SS,
> > -                                  State->GeneralRegs[FAST486_REG_ESP].LowWord,
> > -                                  &ShortValue,
> > -                                  sizeof(USHORT));
> > -    }
> > +    }
> > +
> > +    return TRUE;
> >   }
> >   FORCEINLINE
> >  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev 
--
Alexander Andrejevic <theflash(a)sdf.lonestar.org>
SDF Public Access UNIX System - http://sdf.lonestar.org