Re: [win-pv-devel] [PATCH] Alloc/Free memory for FrontendPath and TargetPath

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: win-pv-devel <win-pv-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf
> Of Owen Smith
> Sent: 15 January 2020 11:11
> To: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Owen Smith <owen.smith@xxxxxxxxxx>
> Subject: [win-pv-devel] [PATCH] Alloc/Free memory for FrontendPath and
> TargetPath
> 
> Using a static sized buffer for FrontendPath triggered failures when
> the DeviceId is longer than 8 characters, as the RtlStringCbPrintfA call
> failed to prefent a buffer overflow.

There should be an upper bound on the size of the id though... 

My reading of 
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/man/xen-vbd-interface.7.pandoc;;hb=HEAD
 says that a decimal representation of:

1 << 28 | 0xfffff << 8 | 0xff

ought to be the limit and by my reckoning that's 9 digits, so I think one extra 
'X' should be enough, shouldn't it? Also, isn't a 4 character target id always 
going to be enough?

  Paul  

> 
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxxx>
> ---
>  src/xenvbd/frontend.c | 63 +++++++++++++++++++++++++++++++++++++++-------
> -----
>  1 file changed, 49 insertions(+), 14 deletions(-)
> 
> diff --git a/src/xenvbd/frontend.c b/src/xenvbd/frontend.c
> index 3fa19a0..f6df88d 100644
> --- a/src/xenvbd/frontend.c
> +++ b/src/xenvbd/frontend.c
> @@ -62,9 +62,9 @@ struct _XENVBD_FRONTEND {
>      PXENVBD_TARGET              Target;
>      ULONG                       TargetId;
>      ULONG                       DeviceId;
> -    CHAR
> FrontendPath[sizeof("device/vbd/XXXXXXXX")];
> +    PCHAR                       FrontendPath;
>      PCHAR                       BackendPath;
> -    CHAR
> TargetPath[sizeof("data/scsi/target/XXXX")];
> +    PCHAR                       TargetPath;
>      USHORT                      BackendDomain;
>      XENVBD_STATE                State;
>      KSPIN_LOCK                  StateLock;
> @@ -1615,7 +1615,7 @@ FrontendDebugCallback(
>      XENBUS_DEBUG(Printf,
>                   &Frontend->DebugInterface,
>                   "FrontendPath: %s\n",
> -                 Frontend->FrontendPath);
> +                 Frontend->FrontendPath ? Frontend->FrontendPath :
> "NULL");
>      XENBUS_DEBUG(Printf,
>                   &Frontend->DebugInterface,
>                   "BackendPath: %s\n",
> @@ -1623,7 +1623,7 @@ FrontendDebugCallback(
>      XENBUS_DEBUG(Printf,
>                   &Frontend->DebugInterface,
>                   "TargetPath: %s\n",
> -                 Frontend->TargetPath);
> +                 Frontend->TargetPath ? Frontend->TargetPath : "NULL");
>      XENBUS_DEBUG(Printf,
>                   &Frontend->DebugInterface,
>                   "State: %s\n",
> @@ -1879,6 +1879,7 @@ FrontendCreate(
>      )
>  {
>      PXENVBD_FRONTEND        Frontend;
> +    ULONG                   Size;
>      NTSTATUS                status;
> 
>      Trace("Target[%d] @ (%d) =====>\n", TargetId, KeGetCurrentIrql());
> @@ -1902,31 +1903,51 @@ FrontendCreate(
>              __FrontendGetTargetId(Frontend),
>              Frontend->MaxQueues);
> 
> +    Size = (ULONG)(strlen("device/vbd/") +
> +                   strlen(DeviceId) +
> +                   1) * sizeof(CHAR);
> +
> +    Frontend->FrontendPath = __FrontendAlloc(Size);
> +
> +    status = STATUS_NO_MEMORY;
> +    if (Frontend->FrontendPath == NULL)
> +        goto fail2;
> +
>      status = RtlStringCbPrintfA(Frontend->FrontendPath,
> -                                sizeof(Frontend->FrontendPath),
> +                                Size,
>                                  "device/vbd/%u",
>                                  Frontend->DeviceId);
>      if (!NT_SUCCESS(status))
> -        goto fail2;
> +        goto fail3;
> +
> +    Size = (ULONG)(strlen("data/scsi/target/") +
> +                   strlen("XXXX") +
> +                   1) * sizeof(CHAR);
> +
> +    Frontend->TargetPath = __FrontendAlloc(Size);
> +
> +    status = STATUS_NO_MEMORY;
> +    if (Frontend->TargetPath == NULL)
> +        goto fail4;
> 
>      status = RtlStringCbPrintfA(Frontend->TargetPath,
>                                  sizeof(Frontend->TargetPath),
>                                  "data/scsi/target/%u",
>                                  TargetId);
>      if (!NT_SUCCESS(status))
> -        goto fail3;
> +        goto fail5;
> 
>      status = RingCreate(Frontend, &Frontend->Ring);
>      if (!NT_SUCCESS(status))
> -        goto fail4;
> +        goto fail6;
> 
>      status = GranterCreate(Frontend, &Frontend->Granter);
>      if (!NT_SUCCESS(status))
> -        goto fail5;
> +        goto fail7;
> 
>      status = ThreadCreate(FrontendBackend, Frontend, &Frontend-
> >BackendThread);
>      if (!NT_SUCCESS(status))
> -        goto fail6;
> +        goto fail8;
> 
>      // kernel objects
>      KeInitializeSpinLock(&Frontend->StateLock);
> @@ -1935,18 +1956,26 @@ FrontendCreate(
>      *_Frontend = Frontend;
>      return STATUS_SUCCESS;
> 
> -fail6:
> -    Error("fail6\n");
> +fail8:
> +    Error("fail8\n");
>      GranterDestroy(Frontend->Granter);
>      Frontend->Granter = NULL;
> -fail5:
> -    Error("fail5\n");
> +fail7:
> +    Error("fail7\n");
>      RingDestroy(Frontend->Ring);
>      Frontend->Ring = NULL;
> +fail6:
> +    Error("fail6\n");
> +fail5:
> +    Error("fail5\n");
> +    __FrontendFree(Frontend->TargetPath);
> +    Frontend->TargetPath = NULL;
>  fail4:
>      Error("fail4\n");
>  fail3:
>      Error("fail3\n");
> +    __FrontendFree(Frontend->FrontendPath);
> +    Frontend->FrontendPath = NULL;
>  fail2:
>      Error("Fail2\n");
>      Frontend->Target = NULL;
> @@ -1991,6 +2020,12 @@ FrontendDestroy(
>      RingDestroy(Frontend->Ring);
>      Frontend->Ring = NULL;
> 
> +    __FrontendFree(Frontend->TargetPath);
> +    Frontend->TargetPath = NULL;
> +
> +    __FrontendFree(Frontend->FrontendPath);
> +    Frontend->FrontendPath = NULL;
> +
>      Frontend->MaxQueues = 0;
> 
>      ASSERT3P(Frontend->BackendPath, ==, NULL);
> --
> 2.16.2.windows.1
> 
> 
> _______________________________________________
> win-pv-devel mailing list
> win-pv-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/win-pv-devel
_______________________________________________
win-pv-devel mailing list
win-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/win-pv-devel