Re: [PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Sebastian Lackner]

On 17.10.2015 14:52, Marcus Meissner wrote:
> 1327477 Wrong sizeof argument
>
> Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> ---
>  dlls/hidclass.sys/device.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> index e7e7e11..dcc7d3c 100644
> --- a/dlls/hidclass.sys/device.c
> +++ b/dlls/hidclass.sys/device.c
> @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
>              packet->reportId = 0;
>
> irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
>                  &irp_status);
>
> irpsp = IoGetNextIrpStackLocation(irp);

This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
might want to review this part again, especially since Coverity detected more issues in this code
(unnecessary assignment of "rc" for example).

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Marcus Meissner]

On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
> On 17.10.2015 14:52, Marcus Meissner wrote:
> > 1327477 Wrong sizeof argument
> >
> > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> > ---
> >  dlls/hidclass.sys/device.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> > index e7e7e11..dcc7d3c 100644
> > --- a/dlls/hidclass.sys/device.c
> > +++ b/dlls/hidclass.sys/device.c
> > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
> >              packet->reportId = 0;
> >
> > irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> > -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> > +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
> >                  &irp_status);
> >
> > irpsp = IoGetNextIrpStackLocation(irp);
>
> This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
> I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
> might want to review this part again, especially since Coverity detected more issues in this code
> (unnecessary assignment of "rc" for example).

something seems wrong though here, yes.

rc issue is "CID 1327478 Unused value"

Ciao, Marcus

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Aric Stewart]

On 10/17/15 8:34 AM, Marcus Meissner wrote:
> On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
> > On 17.10.2015 14:52, Marcus Meissner wrote:
> > > 1327477 Wrong sizeof argument
> > >
> > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> > > ---
> > >   dlls/hidclass.sys/device.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> > > index e7e7e11..dcc7d3c 100644
> > > --- a/dlls/hidclass.sys/device.c
> > > +++ b/dlls/hidclass.sys/device.c
> > > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
> > >               packet->reportId = 0;
> > >
> > > irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> > > -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> > > +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
> > >                   &irp_status);
> > >
> > > irpsp = IoGetNextIrpStackLocation(irp);
> >
> > This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
> > I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
> > might want to review this part again, especially since Coverity detected more issues in this code
> > (unnecessary assignment of "rc" for example).
>
> something seems wrong though here, yes.
>
> rc issue is "CID 1327478 Unused value"
>
> Ciao, Marcus

What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure. 

I am pretty sure Marcus' original patch is unnecessary.  I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft. 

-aric

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Henri Verbeet]

On 19 October 2015 at 15:04, Aric Stewart <aric(a)codeweavers.com> wrote:
> What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.

But it should still be "sizeof(*packet)" instead of "sizeof(packet)", right?

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Aric Stewart]

On 10/19/15 8:27 AM, Henri Verbeet wrote:
> On 19 October 2015 at 15:04, Aric Stewart <aric(a)codeweavers.com> wrote:
> > What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
>
> But it should still be "sizeof(*packet)" instead of "sizeof(packet)", right?

Thanks! This is correct. Patch coming soon.

-aric

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Sebastian Lackner]

On 19.10.2015 15:04, Aric Stewart wrote:
> On 10/17/15 8:34 AM, Marcus Meissner wrote:
> > On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
> > > On 17.10.2015 14:52, Marcus Meissner wrote:
> > > > 1327477 Wrong sizeof argument
> > > >
> > > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> > > > ---
> > > >   dlls/hidclass.sys/device.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> > > > index e7e7e11..dcc7d3c 100644
> > > > --- a/dlls/hidclass.sys/device.c
> > > > +++ b/dlls/hidclass.sys/device.c
> > > > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
> > > >               packet->reportId = 0;
> > > >
> > > > irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> > > > -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> > > > +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
> > > >                   &irp_status);
> > > >
> > > > irpsp = IoGetNextIrpStackLocation(irp);
> > >
> > > This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
> > > I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
> > > might want to review this part again, especially since Coverity detected more issues in this code
> > > (unnecessary assignment of "rc" for example).
> >
> > something seems wrong though here, yes.
> >
> > rc issue is "CID 1327478 Unused value"
> >
> > Ciao, Marcus
>
> What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
>
> I am pretty sure Marcus' original patch is unnecessary.  I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
>
> -aric

I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer,
without any header in front of it, right? Then you would have to pass packet->reportBuffer
(and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because
the buffer will be deallocated automatically after the request is done.

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Aric Stewart]

On 10/19/15 9:08 AM, Sebastian Lackner wrote:
> On 19.10.2015 15:04, Aric Stewart wrote:
> > On 10/17/15 8:34 AM, Marcus Meissner wrote:
> > > On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
> > > > On 17.10.2015 14:52, Marcus Meissner wrote:
> > > > > 1327477 Wrong sizeof argument
> > > > >
> > > > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> > > > > ---
> > > > >    dlls/hidclass.sys/device.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> > > > > index e7e7e11..dcc7d3c 100644
> > > > > --- a/dlls/hidclass.sys/device.c
> > > > > +++ b/dlls/hidclass.sys/device.c
> > > > > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
> > > > >                packet->reportId = 0;
> > > > >
> > > > > irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> > > > > -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> > > > > +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
> > > > >                    &irp_status);
> > > > >
> > > > > irpsp = IoGetNextIrpStackLocation(irp);
> > > >
> > > > This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
> > > > I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
> > > > might want to review this part again, especially since Coverity detected more issues in this code
> > > > (unnecessary assignment of "rc" for example).
> > >
> > > something seems wrong though here, yes.
> > >
> > > rc issue is "CID 1327478 Unused value"
> > >
> > > Ciao, Marcus
> >
> > What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
> >
> > I am pretty sure Marcus' original patch is unnecessary.  I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
> >
> > -aric
>
> I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer,
> without any header in front of it, right? Then you would have to pass packet->reportBuffer
> (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because
> the buffer will be deallocated automatically after the request is done.

The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.

This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85...

The minidriver IOCTL uses the HID_XFER_PACKET structure.

-aric

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Sebastian Lackner]

On 19.10.2015 17:20, Aric Stewart wrote:
> On 10/19/15 9:08 AM, Sebastian Lackner wrote:
> > On 19.10.2015 15:04, Aric Stewart wrote:
> > > On 10/17/15 8:34 AM, Marcus Meissner wrote:
> > > > On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
> > > > > On 17.10.2015 14:52, Marcus Meissner wrote:
> > > > > > 1327477 Wrong sizeof argument
> > > > > >
> > > > > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> > > > > > ---
> > > > > >    dlls/hidclass.sys/device.c | 2 +-
> > > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> > > > > > index e7e7e11..dcc7d3c 100644
> > > > > > --- a/dlls/hidclass.sys/device.c
> > > > > > +++ b/dlls/hidclass.sys/device.c
> > > > > > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
> > > > > >                packet->reportId = 0;
> > > > > >
> > > > > > irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> > > > > > -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> > > > > > +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
> > > > > >                    &irp_status);
> > > > > >
> > > > > > irpsp = IoGetNextIrpStackLocation(irp);
> > > > >
> > > > > This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
> > > > > I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
> > > > > might want to review this part again, especially since Coverity detected more issues in this code
> > > > > (unnecessary assignment of "rc" for example).
> > > >
> > > > something seems wrong though here, yes.
> > > >
> > > > rc issue is "CID 1327478 Unused value"
> > > >
> > > > Ciao, Marcus
> > >
> > > What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
> > >
> > > I am pretty sure Marcus' original patch is unnecessary.  I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
> > >
> > > -aric
> >
> > I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer,
> > without any header in front of it, right? Then you would have to pass packet->reportBuffer
> > (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because
> > the buffer will be deallocated automatically after the request is done.
>
> The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.
>
> This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85...
>
> The minidriver IOCTL uses the HID_XFER_PACKET structure.
>
> -aric

But Henris comment still applies, you want to pass the size of the struct, not the size of a pointer.

[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

[Aric Stewart]

On 10/19/15 10:21 AM, Sebastian Lackner wrote:
> On 19.10.2015 17:20, Aric Stewart wrote:
> > On 10/19/15 9:08 AM, Sebastian Lackner wrote:
> > > On 19.10.2015 15:04, Aric Stewart wrote:
> > > > On 10/17/15 8:34 AM, Marcus Meissner wrote:
> > > > > On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
> > > > > > On 17.10.2015 14:52, Marcus Meissner wrote:
> > > > > > > 1327477 Wrong sizeof argument
> > > > > > >
> > > > > > > Signed-off-by: Marcus Meissner <marcus(a)jet.franken.de>
> > > > > > > ---
> > > > > > >     dlls/hidclass.sys/device.c | 2 +-
> > > > > > >     1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
> > > > > > > index e7e7e11..dcc7d3c 100644
> > > > > > > --- a/dlls/hidclass.sys/device.c
> > > > > > > +++ b/dlls/hidclass.sys/device.c
> > > > > > > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
> > > > > > >                 packet->reportId = 0;
> > > > > > >     
> > > > > > >                 irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
> > > > > > > -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
> > > > > > > +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
> > > > > > >                     &irp_status);
> > > > > > >     
> > > > > > >                 irpsp = IoGetNextIrpStackLocation(irp);
> > > > > > >
> > > > > >
> > > > > > This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
> > > > > > I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
> > > > > > might want to review this part again, especially since Coverity detected more issues in this code
> > > > > > (unnecessary assignment of "rc" for example).
> > > > >
> > > > > something seems wrong though here, yes.
> > > > >
> > > > > rc issue is "CID 1327478 Unused value"
> > > > >
> > > > > Ciao, Marcus
> > > >
> > > > What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
> > > >
> > > > I am pretty sure Marcus' original patch is unnecessary.  I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
> > > >
> > > > -aric
> > >
> > > I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer,
> > > without any header in front of it, right? Then you would have to pass packet->reportBuffer
> > > (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because
> > > the buffer will be deallocated automatically after the request is done.
> >
> > The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.
> >
> > This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85...
> >
> > The minidriver IOCTL uses the HID_XFER_PACKET structure.
> >
> > -aric
>
> But Henris comment still applies, you want to pass the size of the struct, not the size of a pointer.

That is correct. I am working on a patch for that, and the unused assignment. Had an e-mail in composition to that regard, but multitasking pulled me away. :)

-aric