RE: [PATCH 3/3] Fix issues raised by CodeQL

[Owen Smith <owen.smith@xxxxxxxxxx>]

-----Original Message-----
From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of 
Paul Durrant
Sent: Friday, August 20, 2021 9:15 AM
To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Subject: Re: [PATCH 3/3] Fix issues raised by CodeQL

[CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments 
unless you have verified the sender and know the content is safe.

On 10/08/2021 16:40, Owen Smith wrote:
> - ExAllocatePoolWithTag is deprecated in Windows 10 2004 and replaced with
>      ExAllocatePool2. Use ExAllocatePoolUninitialized to maintain support for
>      earlier versions of Windows.
> - strtol can fail, check for LONG_MIN or LONG_MAX as indicators of an 
> error

I think these probably ought to be separate patches. As for the strtol() 
change... There also uses of strtoul() (and _strtoui64()) elsewhere. If we're 
going to catch overflow here then ought we not have similar checking there?

   Paul

Only strtol() was flagged as a warning by the newer compiler, not strtoul() or 
_strtoui64(). I only fixed the minimum to get the compile to stop raising 
warnings

Owen

> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>   src/common/util.h      |  4 ++++
>   src/xenfilt/emulated.c | 34 ++++++++++++++++++++++++++--------
>   2 files changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/src/common/util.h b/src/common/util.h index 
> eddad4a..36a36dd 100644
> --- a/src/common/util.h
> +++ b/src/common/util.h
> @@ -151,8 +151,12 @@ __AllocatePoolWithTag(
>       __analysis_assume(PoolType == NonPagedPool ||
>                         PoolType == PagedPool);
>   
> +#if (_MSC_VER >= 1928) // VS 16.9 (EWDK 20344 or later)
> +    Buffer = ExAllocatePoolUninitialized(PoolType, NumberOfBytes, 
> +Tag); #else
>   #pragma warning(suppress:28160) // annotation error
>       Buffer = ExAllocatePoolWithTag(PoolType, NumberOfBytes, Tag);
> +#endif
>       if (Buffer == NULL)
>           return NULL;
>   
> diff --git a/src/xenfilt/emulated.c b/src/xenfilt/emulated.c index 
> b7ae510..c0b666e 100644
> --- a/src/xenfilt/emulated.c
> +++ b/src/xenfilt/emulated.c
> @@ -159,9 +159,9 @@ EmulatedSetObjectDiskData(
>       )
>   {
>       PCHAR                               End;
> -    ULONG                               Controller;
> -    ULONG                               Target;
> -    ULONG                               Lun;
> +    LONG                                Controller;
> +    LONG                                Target;
> +    LONG                                Lun;
>       NTSTATUS                            status;
>   
>       UNREFERENCED_PARAMETER(DeviceID); @@ -171,36 +171,54 @@ 
> EmulatedSetObjectDiskData(
>       if (Type != XENFILT_EMULATED_OBJECT_TYPE_IDE)
>           goto fail1;
>   
> +    status = STATUS_INVALID_PARAMETER;
>       Controller = strtol(InstanceID, &End, 10);
> +    if (Controller == LONG_MIN || Controller == LONG_MAX)
> +        goto fail2;
>   
>       status = STATUS_INVALID_PARAMETER;
>       if (*End != '.' || Controller > 1)
> -        goto fail2;
> +        goto fail3;
>   
>       End++;
>   
> +    status = STATUS_INVALID_PARAMETER;
>       Target = strtol(End, &End, 10);
> +    if (Target == LONG_MIN || Target == LONG_MAX)
> +        goto fail4;
>   
>       status = STATUS_INVALID_PARAMETER;
>       if (*End != '.' || Target > 1)
> -        goto fail3;
> +        goto fail5;
>   
>       End++;
>   
> +    status = STATUS_INVALID_PARAMETER;
>       Lun = strtol(End, &End, 10);
> +    if (Lun == LONG_MIN || Lun == LONG_MAX)
> +        goto fail6;
>   
>       status = STATUS_INVALID_PARAMETER;
>       if (*End != '\0')
> -        goto fail4;
> +        goto fail7;
>   
>       status = STATUS_NOT_SUPPORTED;
>       if (Lun != 0)
> -        goto fail5;
> +        goto fail8;
>   
> -    EmulatedObject->Data.Disk.Index = Controller << 1 | Target;
> +    EmulatedObject->Data.Disk.Index = (ULONG)Controller << 1 | 
> + (ULONG)Target;
>   
>       return STATUS_SUCCESS;
>   
> +fail8:
> +    Error("fail8\n");
> +
> +fail7:
> +    Error("fail7\n");
> +
> +fail6:
> +    Error("fail6\n");
> +
>   fail5:
>       Error("fail5\n");
>   
>