Re: ntoskrnl.exe: Make IoAllocateIrp not crash on negative values.

[Sebastian Lackner]

On 27.03.2016 20:27, Bernhard Übelacker wrote:
> https://bugs.winehq.org/show_bug.cgi?id=39734
>
> Changes should avoid crash in acedrv11.sys.
> IoAllocateIrp is called with a stack_size of -128.
>
> Tested against Windows XP.
> (See the test based on wine-staging "driver testing framework".)

For reference, here a testbot run which confirms that the change is correct (for XP/2003):
https://newtestbot.winehq.org/JobDetails.pl?Key=21722
> Signed-off-by: Bernhard Übelacker <bernhardu(a)vr-web.de>
> ---
>  dlls/ntoskrnl.exe/ntoskrnl.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> index 36488a7..f2ccc61 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> @@ -592,15 +592,20 @@ PIRP WINAPI IoAllocateIrp( CCHAR stack_size, BOOLEAN charge_quota )
>  {
>      SIZE_T size;
>      PIRP irp;
> +    CCHAR _stack_size = stack_size;
>
> TRACE( "%d, %d\n", stack_size, charge_quota );
>
> -    size = sizeof(IRP) + stack_size * sizeof(IO_STACK_LOCATION);
> +    if (_stack_size <= 0 || (_stack_size > 1 && stack_size < 8))
> +        _stack_size = 8;

I would suggest to use a different variable, its very easy to mix up "stack_size"
and "_stack_size". You even mixed it up yourself in the if() condition above. ;)
If you want, you can also simplify the condition to (stack_size < 8 && stack_size != 1).
> +
> +    size = sizeof(IRP) + _stack_size * sizeof(IO_STACK_LOCATION);
>      irp = ExAllocatePool( NonPagedPool, size );
>      if (irp == NULL)
>          return NULL;
>      IoInitializeIrp( irp, size, stack_size );
> -    irp->AllocationFlags = IRP_ALLOCATED_FIXED_SIZE;
> +    if (stack_size >= 1 && stack_size <= 8)
> +        irp->AllocationFlags = IRP_ALLOCATED_FIXED_SIZE;
>      if (charge_quota)
>          irp->AllocationFlags |= IRP_LOOKASIDE_ALLOCATION;
>      return irp;

ntoskrnl.exe: Make IoAllocateIrp not crash on negative values.

[Bernhard Übelacker]

Hello Sebastian,

Am 28.03.2016 um 08:02 schrieb Sebastian Lackner:
> On 27.03.2016 20:27, Bernhard Übelacker wrote:
> > https://bugs.winehq.org/show_bug.cgi?id=39734
> >
> > Changes should avoid crash in acedrv11.sys.
> > IoAllocateIrp is called with a stack_size of -128.
> >
> > Tested against Windows XP.
> > (See the test based on wine-staging "driver testing framework".)
>
> For reference, here a testbot run which confirms that the change is correct (for XP/2003):
> https://newtestbot.winehq.org/JobDetails.pl?Key=21722
> > Signed-off-by: Bernhard Übelacker <bernhardu(a)vr-web.de>
> > ---
> >   dlls/ntoskrnl.exe/ntoskrnl.c | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> > index 36488a7..f2ccc61 100644
> > --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> > +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> > @@ -592,15 +592,20 @@ PIRP WINAPI IoAllocateIrp( CCHAR stack_size, BOOLEAN charge_quota )
> >   {
> >       SIZE_T size;
> >       PIRP irp;
> > +    CCHAR _stack_size = stack_size;
> >
> > TRACE( "%d, %d\n", stack_size, charge_quota );
> >
> > -    size = sizeof(IRP) + stack_size * sizeof(IO_STACK_LOCATION);
> > +    if (_stack_size <= 0 || (_stack_size > 1 && stack_size < 8))
> > +        _stack_size = 8;
>
> I would suggest to use a different variable, its very easy to mix up "stack_size"
> and "_stack_size". You even mixed it up yourself in the if() condition above. ;)
> If you want, you can also simplify the condition to (stack_size < 8 && stack_size != 1).
> > +
> > +    size = sizeof(IRP) + _stack_size * sizeof(IO_STACK_LOCATION);
> >       irp = ExAllocatePool( NonPagedPool, size );
> >       if (irp == NULL)
> >           return NULL;
> >       IoInitializeIrp( irp, size, stack_size );
> > -    irp->AllocationFlags = IRP_ALLOCATED_FIXED_SIZE;
> > +    if (stack_size >= 1 && stack_size <= 8)
> > +        irp->AllocationFlags = IRP_ALLOCATED_FIXED_SIZE;
> >       if (charge_quota)
> >           irp->AllocationFlags |= IRP_LOOKASIDE_ALLOCATION;
> >       return irp;

thanks for your review.
I have sent an updated patch.

Is the test attached to the bug worth to be included into wine-staging?

Kind regards,
Bernhard