Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-15 Thread Kinney, Michael D 
Star,

I agree they are optional.  The description says
DataLength should be 0 and Data should be NULL for
the EfiUsbNoData case.  That is why I did not think
Direction needed to be evaluated if DataLength must
be 0 in this case.

In fact, the Host Controller implementations return
an error if this is not the case:

  if ((TransferDirection == EfiUsbNoData) &&
  ((Data != NULL) || (*DataLength != 0))) {
return EFI_INVALID_PARAMETER;
  }

So I think the logic in the current patch is safe.

However, since there are questions on this topic,
I agree I can update the logic to only check for
short data if Direction is not EfiUsbNoData.  That
should help reduce questions in future reviews of
this function.  I will also add more comments.

Thanks for the feedback.

Mike
 
> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, November 15, 2017 6:26 PM
> To: Kinney, Michael D ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star
> 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> Yes, the DataLength could be read. And Data/DataLength
> is optional when Direction is EfiUsbNoData per my
> understanding.
> 
>   IN  OUT VOID*Data,  OPTIONAL
>   IN  UINTN   DataLength, OPTIONAL
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Thursday, November 16, 2017 12:44 AM
> To: Zeng, Star ; edk2-
> de...@lists.01.org; Kinney, Michael D
> 
> Cc: Dong, Eric 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Star,
> 
> I have read that section, and I do not see anything
> that says the DataLength parameter can not be read.
> 
> Mike
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 8:23 PM
> > To: Kinney, Michael D ;
> > edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star
> 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Mike,
> >
> > UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(),
> in the
> > description.
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Kinney, Michael D
> > Sent: Wednesday, November 15, 2017 12:00 PM
> > To: Zeng, Star ; edk2-
> de...@lists.01.org;
> > Kinney, Michael D 
> > Cc: Dong, Eric 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Star,
> >
> > I am curious.  Which spec statement are you referring
> to?
> >
> > Mike
> >
> > > -Original Message-
> > > From: Zeng, Star
> > > Sent: Tuesday, November 14, 2017 6:59 PM
> > > To: Kinney, Michael D ;
> > > edk2-devel@lists.01.org
> > > Cc: Dong, Eric ; Zeng, Star
> > 
> > > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> > Add
> > > UsbControlTransfer() error check
> > >
> > > Mike,
> > >
> > > I do not insist on the check that the Direction is
> > not EfiUsbNoData,
> > > and I know the functionality should have no
> > improvement with the
> > > check.
> > > So, you can have my Reviewed-by: Star Zeng
> > .
> > >
> > > I raised the question just for consideration from
> > literally, as
> > > according to the spec, the code could not touch
> > DataLength at all when
> > > Direction is EfiUsbNoData.
> > >
> > > You can decide to have / have not the check when
> > pushing. :)
> > >
> > >
> > > Thanks,
> > > Star
> > > -Original Message-
> > > From: Kinney, Michael D
> > > Sent: Wednesday, November 15, 2017 10:46 AM
> > > To: Zeng, Star ; edk2-
> > de...@lists.01.org;
> > > Kinney, Michael D 
> > > Cc: Dong, Eric 
> > > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> > Add
> > > UsbControlTransfer() error check
> > >
> > > Star,
> > >
> > > For that case both DataLength and
> RequestedDataLength
> > will be 0 and
> > > the new path will not be taken.
> > >
> > > Are you concerned that the USB HC Protocol could
> > return a non zero
> > > DataLength for the EfiUsbNoData case?  Even if it
> > does, the new path
> > > can never be taken because 0 is less than all UINTN
> > values.
> > >
> > > Mike
> > >
> > > > -Original Message-
> > > > From: Zeng, Star
> > > > Sent: Tuesday, November 14, 2017 6:40 PM
> > > > To: Kinney, Michael D
> ;
> > > > edk2-devel@lists.01.org
> > > > Cc: Dong, Eric ; Zeng, Star
> > > 
> > > > Subject: RE: [Patch V2 1/2]
> MdeModulePkg/UsbBusDxe:
> > > Add
> > > > UsbControlTransfer() error check
> > > >
> > > > Mike,
> > > >

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-15 Thread Zeng, Star 
Mike,

Yes, the DataLength could be read. And Data/DataLength is optional when 
Direction is EfiUsbNoData per my understanding.

  IN  OUT VOID*Data,  OPTIONAL
  IN  UINTN   DataLength, OPTIONAL


Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Thursday, November 16, 2017 12:44 AM
To: Zeng, Star ; edk2-devel@lists.01.org; Kinney, Michael 
D 
Cc: Dong, Eric 
Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() 
error check

Star,

I have read that section, and I do not see anything that says the DataLength 
parameter can not be read.

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 8:23 PM
> To: Kinney, Michael D ; 
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(), in the 
> description.
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 12:00 PM
> To: Zeng, Star ; edk2- de...@lists.01.org; 
> Kinney, Michael D 
> Cc: Dong, Eric 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Star,
> 
> I am curious.  Which spec statement are you referring to?
> 
> Mike
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 6:59 PM
> > To: Kinney, Michael D ; 
> > edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star
> 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Mike,
> >
> > I do not insist on the check that the Direction is
> not EfiUsbNoData,
> > and I know the functionality should have no
> improvement with the
> > check.
> > So, you can have my Reviewed-by: Star Zeng
> .
> >
> > I raised the question just for consideration from
> literally, as
> > according to the spec, the code could not touch
> DataLength at all when
> > Direction is EfiUsbNoData.
> >
> > You can decide to have / have not the check when
> pushing. :)
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Kinney, Michael D
> > Sent: Wednesday, November 15, 2017 10:46 AM
> > To: Zeng, Star ; edk2-
> de...@lists.01.org;
> > Kinney, Michael D 
> > Cc: Dong, Eric 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Star,
> >
> > For that case both DataLength and RequestedDataLength
> will be 0 and
> > the new path will not be taken.
> >
> > Are you concerned that the USB HC Protocol could
> return a non zero
> > DataLength for the EfiUsbNoData case?  Even if it
> does, the new path
> > can never be taken because 0 is less than all UINTN
> values.
> >
> > Mike
> >
> > > -Original Message-
> > > From: Zeng, Star
> > > Sent: Tuesday, November 14, 2017 6:40 PM
> > > To: Kinney, Michael D ; 
> > > edk2-devel@lists.01.org
> > > Cc: Dong, Eric ; Zeng, Star
> > 
> > > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> > Add
> > > UsbControlTransfer() error check
> > >
> > > Mike,
> > >
> > > Do you think it is better or not to also check the
> > Direction is not
> > > EfiUsbNoData?
> > >
> > > UEFI spec,
> EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> > > If the Direction parameter is EfiUsbNoData, Data is
> > NULL, and
> > > DataLength is 0, then no data phase exists for the
> > control transfer.
> > >
> > > Thanks,
> > > Star
> > > -Original Message-
> > > From: Kinney, Michael D
> > > Sent: Wednesday, November 15, 2017 9:03 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Zeng, Star ; Dong, Eric
> > 
> > > Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> > > UsbControlTransfer() error check
> > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=767
> > >
> > > The USB I/O Protocol function ControlTransfer() has
> a
> > DataLength
> > > parameter that specifies the size of the Data
> buffer.
> > The UsbBusDxe
> > > module implements the USB I/O Protocol using the
> > services of the USB2
> > > Host Controller Protocol.  The DataLength parameter
> > in the
> > > USB2 Host Controller Protocol ControlTransfer()
> > service is an IN OUT
> > > parameter so the number of bytes actually
> transferred
> > is returned.
> > > Since the USB I/O Protocol
> > > ControlTransfer() service can not return the number
> > of bytes actually
> > > transferred, the only option if the number of bytes
> > actually
> > >

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-15 Thread Kinney, Michael D 
Star,

I have read that section, and I do not see anything
that says the DataLength parameter can not be read.

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 8:23 PM
> To: Kinney, Michael D ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star
> 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(), in
> the description.
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 12:00 PM
> To: Zeng, Star ; edk2-
> de...@lists.01.org; Kinney, Michael D
> 
> Cc: Dong, Eric 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Star,
> 
> I am curious.  Which spec statement are you referring
> to?
> 
> Mike
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 6:59 PM
> > To: Kinney, Michael D ;
> > edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star
> 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Mike,
> >
> > I do not insist on the check that the Direction is
> not EfiUsbNoData,
> > and I know the functionality should have no
> improvement with the
> > check.
> > So, you can have my Reviewed-by: Star Zeng
> .
> >
> > I raised the question just for consideration from
> literally, as
> > according to the spec, the code could not touch
> DataLength at all when
> > Direction is EfiUsbNoData.
> >
> > You can decide to have / have not the check when
> pushing. :)
> >
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Kinney, Michael D
> > Sent: Wednesday, November 15, 2017 10:46 AM
> > To: Zeng, Star ; edk2-
> de...@lists.01.org;
> > Kinney, Michael D 
> > Cc: Dong, Eric 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Star,
> >
> > For that case both DataLength and RequestedDataLength
> will be 0 and
> > the new path will not be taken.
> >
> > Are you concerned that the USB HC Protocol could
> return a non zero
> > DataLength for the EfiUsbNoData case?  Even if it
> does, the new path
> > can never be taken because 0 is less than all UINTN
> values.
> >
> > Mike
> >
> > > -Original Message-
> > > From: Zeng, Star
> > > Sent: Tuesday, November 14, 2017 6:40 PM
> > > To: Kinney, Michael D ;
> > > edk2-devel@lists.01.org
> > > Cc: Dong, Eric ; Zeng, Star
> > 
> > > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> > Add
> > > UsbControlTransfer() error check
> > >
> > > Mike,
> > >
> > > Do you think it is better or not to also check the
> > Direction is not
> > > EfiUsbNoData?
> > >
> > > UEFI spec,
> EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> > > If the Direction parameter is EfiUsbNoData, Data is
> > NULL, and
> > > DataLength is 0, then no data phase exists for the
> > control transfer.
> > >
> > > Thanks,
> > > Star
> > > -Original Message-
> > > From: Kinney, Michael D
> > > Sent: Wednesday, November 15, 2017 9:03 AM
> > > To: edk2-devel@lists.01.org
> > > Cc: Zeng, Star ; Dong, Eric
> > 
> > > Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> > > UsbControlTransfer() error check
> > >
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=767
> > >
> > > The USB I/O Protocol function ControlTransfer() has
> a
> > DataLength
> > > parameter that specifies the size of the Data
> buffer.
> > The UsbBusDxe
> > > module implements the USB I/O Protocol using the
> > services of the USB2
> > > Host Controller Protocol.  The DataLength parameter
> > in the
> > > USB2 Host Controller Protocol ControlTransfer()
> > service is an IN OUT
> > > parameter so the number of bytes actually
> transferred
> > is returned.
> > > Since the USB I/O Protocol
> > > ControlTransfer() service can not return the number
> > of bytes actually
> > > transferred, the only option if the number of bytes
> > actually
> > > transferred is less than the number of bytes
> > requested is to return
> > > EFI_DEVICE_ERROR.
> > >
> > > The change fixes an issue with a USB mass storage
> > device that responds
> > > with 0 bytes to the Get MAX LUN command.
> > >
> > > Cc: Star Zeng 
> > > Cc: Eric Dong 
> > > Contributed-under: TianoCore Contribution Agreement
> > 1.1
> > > Signed-off-by: Michael D Kinney
> > > 
> > > ---
> > >  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8
> +++-
> > >  1 file changed, 7 insertions(+), 1

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-14 Thread Zeng, Star 
Mike,

UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer(), in the description.

Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Wednesday, November 15, 2017 12:00 PM
To: Zeng, Star ; edk2-devel@lists.01.org; Kinney, Michael 
D 
Cc: Dong, Eric 
Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() 
error check

Star,

I am curious.  Which spec statement are you referring to?

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:59 PM
> To: Kinney, Michael D ; 
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> I do not insist on the check that the Direction is not EfiUsbNoData, 
> and I know the functionality should have no improvement with the 
> check.
> So, you can have my Reviewed-by: Star Zeng .
> 
> I raised the question just for consideration from literally, as 
> according to the spec, the code could not touch DataLength at all when 
> Direction is EfiUsbNoData.
> 
> You can decide to have / have not the check when pushing. :)
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 10:46 AM
> To: Zeng, Star ; edk2- de...@lists.01.org; 
> Kinney, Michael D 
> Cc: Dong, Eric 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Star,
> 
> For that case both DataLength and RequestedDataLength will be 0 and 
> the new path will not be taken.
> 
> Are you concerned that the USB HC Protocol could return a non zero 
> DataLength for the EfiUsbNoData case?  Even if it does, the new path 
> can never be taken because 0 is less than all UINTN values.
> 
> Mike
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 6:40 PM
> > To: Kinney, Michael D ; 
> > edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star
> 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Mike,
> >
> > Do you think it is better or not to also check the
> Direction is not
> > EfiUsbNoData?
> >
> > UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> > If the Direction parameter is EfiUsbNoData, Data is
> NULL, and
> > DataLength is 0, then no data phase exists for the
> control transfer.
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Kinney, Michael D
> > Sent: Wednesday, November 15, 2017 9:03 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Dong, Eric
> 
> > Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> > UsbControlTransfer() error check
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=767
> >
> > The USB I/O Protocol function ControlTransfer() has a
> DataLength
> > parameter that specifies the size of the Data buffer.
> The UsbBusDxe
> > module implements the USB I/O Protocol using the
> services of the USB2
> > Host Controller Protocol.  The DataLength parameter
> in the
> > USB2 Host Controller Protocol ControlTransfer()
> service is an IN OUT
> > parameter so the number of bytes actually transferred
> is returned.
> > Since the USB I/O Protocol
> > ControlTransfer() service can not return the number
> of bytes actually
> > transferred, the only option if the number of bytes
> actually
> > transferred is less than the number of bytes
> requested is to return
> > EFI_DEVICE_ERROR.
> >
> > The change fixes an issue with a USB mass storage
> device that responds
> > with 0 bytes to the Get MAX LUN command.
> >
> > Cc: Star Zeng 
> > Cc: Eric Dong 
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Michael D Kinney
> > 
> > ---
> >  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > index 78220222f6..b0ad7938e9 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > @@ -2,7 +2,7 @@
> >
> >  Usb Bus Driver Binding and Bus IO Protocol.
> >
> > -Copyright (c) 2004 - 2016, Intel Corporation. All
> rights
> > reserved.
> > +Copyright (c) 2004 - 2017, Intel Corporation. All
> > rights reserved.
> >  This program and the accompanying materials  are
> licensed and made
> > available under the terms and conditions of the BSD
> License  which
> > accompanies this distribution.  The full text of the
> license may be
> > found at @@ -76,6 +76,7 @@

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-14 Thread Kinney, Michael D 
Star,

I am curious.  Which spec statement are you referring to?

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:59 PM
> To: Kinney, Michael D ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star
> 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> I do not insist on the check that the Direction is not
> EfiUsbNoData, and I know the functionality should have
> no improvement with the check.
> So, you can have my Reviewed-by: Star Zeng
> .
> 
> I raised the question just for consideration from
> literally, as according to the spec, the code could not
> touch DataLength at all when Direction is EfiUsbNoData.
> 
> You can decide to have / have not the check when
> pushing. :)
> 
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 10:46 AM
> To: Zeng, Star ; edk2-
> de...@lists.01.org; Kinney, Michael D
> 
> Cc: Dong, Eric 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Star,
> 
> For that case both DataLength and RequestedDataLength
> will be 0 and the new path will not be taken.
> 
> Are you concerned that the USB HC Protocol could return
> a non zero DataLength for the EfiUsbNoData case?  Even
> if it does, the new path can never be taken because 0
> is less than all UINTN values.
> 
> Mike
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Tuesday, November 14, 2017 6:40 PM
> > To: Kinney, Michael D ;
> > edk2-devel@lists.01.org
> > Cc: Dong, Eric ; Zeng, Star
> 
> > Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe:
> Add
> > UsbControlTransfer() error check
> >
> > Mike,
> >
> > Do you think it is better or not to also check the
> Direction is not
> > EfiUsbNoData?
> >
> > UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> > If the Direction parameter is EfiUsbNoData, Data is
> NULL, and
> > DataLength is 0, then no data phase exists for the
> control transfer.
> >
> > Thanks,
> > Star
> > -Original Message-
> > From: Kinney, Michael D
> > Sent: Wednesday, November 15, 2017 9:03 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Dong, Eric
> 
> > Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> > UsbControlTransfer() error check
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=767
> >
> > The USB I/O Protocol function ControlTransfer() has a
> DataLength
> > parameter that specifies the size of the Data buffer.
> The UsbBusDxe
> > module implements the USB I/O Protocol using the
> services of the USB2
> > Host Controller Protocol.  The DataLength parameter
> in the
> > USB2 Host Controller Protocol ControlTransfer()
> service is an IN OUT
> > parameter so the number of bytes actually transferred
> is returned.
> > Since the USB I/O Protocol
> > ControlTransfer() service can not return the number
> of bytes actually
> > transferred, the only option if the number of bytes
> actually
> > transferred is less than the number of bytes
> requested is to return
> > EFI_DEVICE_ERROR.
> >
> > The change fixes an issue with a USB mass storage
> device that responds
> > with 0 bytes to the Get MAX LUN command.
> >
> > Cc: Star Zeng 
> > Cc: Eric Dong 
> > Contributed-under: TianoCore Contribution Agreement
> 1.1
> > Signed-off-by: Michael D Kinney
> > 
> > ---
> >  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > index 78220222f6..b0ad7938e9 100644
> > --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> > @@ -2,7 +2,7 @@
> >
> >  Usb Bus Driver Binding and Bus IO Protocol.
> >
> > -Copyright (c) 2004 - 2016, Intel Corporation. All
> rights
> > reserved.
> > +Copyright (c) 2004 - 2017, Intel Corporation. All
> > rights reserved.
> >  This program and the accompanying materials  are
> licensed and made
> > available under the terms and conditions of the BSD
> License  which
> > accompanies this distribution.  The full text of the
> license may be
> > found at @@ -76,6 +76,7 @@ UsbIoControlTransfer (
> >USB_ENDPOINT_DESC   *EpDesc;
> >EFI_TPL OldTpl;
> >EFI_STATUS  Status;
> > +  UINTN   RequestedDataLength;
> >
> >if (UsbStatus == NULL) {
> >  return EFI_INVALID_PARAMETER;
> > @@ -86,6 +87,7 @@ UsbIoControlTransfer (
> >UsbIf  = USB_INTERFACE_FROM_USBIO (This);
> >Dev= UsbIf->Device;
> >
> > +  RequestedDataLength = DataLength;
> >

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-14 Thread Zeng, Star 
Mike,

I do not insist on the check that the Direction is not EfiUsbNoData, and I know 
the functionality should have no improvement with the check.
So, you can have my Reviewed-by: Star Zeng .

I raised the question just for consideration from literally, as according to 
the spec, the code could not touch DataLength at all when Direction is 
EfiUsbNoData.

You can decide to have / have not the check when pushing. :)


Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Wednesday, November 15, 2017 10:46 AM
To: Zeng, Star ; edk2-devel@lists.01.org; Kinney, Michael 
D 
Cc: Dong, Eric 
Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() 
error check

Star,

For that case both DataLength and RequestedDataLength will be 0 and the new 
path will not be taken.

Are you concerned that the USB HC Protocol could return a non zero DataLength 
for the EfiUsbNoData case?  Even if it does, the new path can never be taken 
because 0 is less than all UINTN values.

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:40 PM
> To: Kinney, Michael D ; 
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> Do you think it is better or not to also check the Direction is not 
> EfiUsbNoData?
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> If the Direction parameter is EfiUsbNoData, Data is NULL, and 
> DataLength is 0, then no data phase exists for the control transfer.
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 9:03 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric 
> Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=767
> 
> The USB I/O Protocol function ControlTransfer() has a DataLength 
> parameter that specifies the size of the Data buffer.  The UsbBusDxe 
> module implements the USB I/O Protocol using the services of the USB2 
> Host Controller Protocol.  The DataLength parameter in the
> USB2 Host Controller Protocol ControlTransfer() service is an IN OUT 
> parameter so the number of bytes actually transferred is returned.  
> Since the USB I/O Protocol
> ControlTransfer() service can not return the number of bytes actually 
> transferred, the only option if the number of bytes actually 
> transferred is less than the number of bytes requested is to return 
> EFI_DEVICE_ERROR.
> 
> The change fixes an issue with a USB mass storage device that responds 
> with 0 bytes to the Get MAX LUN command.
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney
> 
> ---
>  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> index 78220222f6..b0ad7938e9 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> @@ -2,7 +2,7 @@
> 
>  Usb Bus Driver Binding and Bus IO Protocol.
> 
> -Copyright (c) 2004 - 2016, Intel Corporation. All rights 
> reserved.
> +Copyright (c) 2004 - 2017, Intel Corporation. All
> rights reserved.
>  This program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License  which 
> accompanies this distribution.  The full text of the license may be 
> found at @@ -76,6 +76,7 @@ UsbIoControlTransfer (
>USB_ENDPOINT_DESC   *EpDesc;
>EFI_TPL OldTpl;
>EFI_STATUS  Status;
> +  UINTN   RequestedDataLength;
> 
>if (UsbStatus == NULL) {
>  return EFI_INVALID_PARAMETER;
> @@ -86,6 +87,7 @@ UsbIoControlTransfer (
>UsbIf  = USB_INTERFACE_FROM_USBIO (This);
>Dev= UsbIf->Device;
> 
> +  RequestedDataLength = DataLength;
>Status = UsbHcControlTransfer (
>   Dev->Bus,
>   Dev->Address,
> @@ -99,6 +101,10 @@ UsbIoControlTransfer (
>   >Translator,
>   UsbStatus
>   );
> +  if (!EFI_ERROR (Status) && DataLength <
> RequestedDataLength) {
> +Status = EFI_DEVICE_ERROR;
> +goto ON_EXIT;
> +  }
> 
>if (EFI_ERROR (Status) || (*UsbStatus !=
> EFI_USB_NOERROR)) {
>  //
> --
> 2.14.2.windows.3

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-14 Thread Kinney, Michael D 
Star,

For that case both DataLength and RequestedDataLength
will be 0 and the new path will not be taken.

Are you concerned that the USB HC Protocol could return
a non zero DataLength for the EfiUsbNoData case?  Even
if it does, the new path can never be taken because 0 is
less than all UINTN values.

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, November 14, 2017 6:40 PM
> To: Kinney, Michael D ;
> edk2-devel@lists.01.org
> Cc: Dong, Eric ; Zeng, Star
> 
> Subject: RE: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> Mike,
> 
> Do you think it is better or not to also check the
> Direction is not EfiUsbNoData?
> 
> UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer():
> If the Direction parameter is EfiUsbNoData, Data is
> NULL, and DataLength is 0, then no data phase exists
> for the control transfer.
> 
> Thanks,
> Star
> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, November 15, 2017 9:03 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric
> 
> Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add
> UsbControlTransfer() error check
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=767
> 
> The USB I/O Protocol function ControlTransfer() has a
> DataLength parameter that specifies the size of the
> Data buffer.  The UsbBusDxe module implements the USB
> I/O Protocol using the services of the USB2 Host
> Controller Protocol.  The DataLength parameter in the
> USB2 Host Controller Protocol ControlTransfer() service
> is an IN OUT parameter so the number of bytes actually
> transferred is returned.  Since the USB I/O Protocol
> ControlTransfer() service can not return the number of
> bytes actually transferred, the only option if the
> number of bytes actually transferred is less than the
> number of bytes requested is to return
> EFI_DEVICE_ERROR.
> 
> The change fixes an issue with a USB mass storage
> device that responds with 0 bytes to the Get MAX LUN
> command.
> 
> Cc: Star Zeng 
> Cc: Eric Dong 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney
> 
> ---
>  MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> index 78220222f6..b0ad7938e9 100644
> --- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> +++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
> @@ -2,7 +2,7 @@
> 
>  Usb Bus Driver Binding and Bus IO Protocol.
> 
> -Copyright (c) 2004 - 2016, Intel Corporation. All
> rights reserved.
> +Copyright (c) 2004 - 2017, Intel Corporation. All
> rights reserved.
>  This program and the accompanying materials  are
> licensed and made available under the terms and
> conditions of the BSD License  which accompanies this
> distribution.  The full text of the license may be
> found at @@ -76,6 +76,7 @@ UsbIoControlTransfer (
>USB_ENDPOINT_DESC   *EpDesc;
>EFI_TPL OldTpl;
>EFI_STATUS  Status;
> +  UINTN   RequestedDataLength;
> 
>if (UsbStatus == NULL) {
>  return EFI_INVALID_PARAMETER;
> @@ -86,6 +87,7 @@ UsbIoControlTransfer (
>UsbIf  = USB_INTERFACE_FROM_USBIO (This);
>Dev= UsbIf->Device;
> 
> +  RequestedDataLength = DataLength;
>Status = UsbHcControlTransfer (
>   Dev->Bus,
>   Dev->Address,
> @@ -99,6 +101,10 @@ UsbIoControlTransfer (
>   >Translator,
>   UsbStatus
>   );
> +  if (!EFI_ERROR (Status) && DataLength <
> RequestedDataLength) {
> +Status = EFI_DEVICE_ERROR;
> +goto ON_EXIT;
> +  }
> 
>if (EFI_ERROR (Status) || (*UsbStatus !=
> EFI_USB_NOERROR)) {
>  //
> --
> 2.14.2.windows.3

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-14 Thread Zeng, Star 
Mike,

Do you think it is better or not to also check the Direction is not 
EfiUsbNoData?

UEFI spec, EFI_USB_IO_PROTOCOL.UsbControlTransfer():
If the Direction parameter is EfiUsbNoData, Data is NULL, and DataLength is 0, 
then no data phase exists for the control transfer.

Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Wednesday, November 15, 2017 9:03 AM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric 
Subject: [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error 
check

https://bugzilla.tianocore.org/show_bug.cgi?id=767

The USB I/O Protocol function ControlTransfer() has a DataLength parameter that 
specifies the size of the Data buffer.  The UsbBusDxe module implements the USB 
I/O Protocol using the services of the USB2 Host Controller Protocol.  The 
DataLength parameter in the USB2 Host Controller Protocol ControlTransfer() 
service is an IN OUT parameter so the number of bytes actually transferred is 
returned.  Since the USB I/O Protocol
ControlTransfer() service can not return the number of bytes actually 
transferred, the only option if the number of bytes actually transferred is 
less than the number of bytes requested is to return EFI_DEVICE_ERROR.

The change fixes an issue with a USB mass storage device that responds with 0 
bytes to the Get MAX LUN command.

Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
index 78220222f6..b0ad7938e9 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
@@ -2,7 +2,7 @@
 
 Usb Bus Driver Binding and Bus IO Protocol.
 
-Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.
 This program and the accompanying materials  are licensed and made available 
under the terms and conditions of the BSD License  which accompanies this 
distribution.  The full text of the license may be found at @@ -76,6 +76,7 @@ 
UsbIoControlTransfer (
   USB_ENDPOINT_DESC   *EpDesc;
   EFI_TPL OldTpl;
   EFI_STATUS  Status;
+  UINTN   RequestedDataLength;
 
   if (UsbStatus == NULL) {
 return EFI_INVALID_PARAMETER;
@@ -86,6 +87,7 @@ UsbIoControlTransfer (
   UsbIf  = USB_INTERFACE_FROM_USBIO (This);
   Dev= UsbIf->Device;
 
+  RequestedDataLength = DataLength;
   Status = UsbHcControlTransfer (
  Dev->Bus,
  Dev->Address,
@@ -99,6 +101,10 @@ UsbIoControlTransfer (
  >Translator,
  UsbStatus
  );
+  if (!EFI_ERROR (Status) && DataLength < RequestedDataLength) {
+Status = EFI_DEVICE_ERROR;
+goto ON_EXIT;
+  }
 
   if (EFI_ERROR (Status) || (*UsbStatus != EFI_USB_NOERROR)) {
 //
--
2.14.2.windows.3

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch V2 1/2] MdeModulePkg/UsbBusDxe: Add UsbControlTransfer() error check

2017-11-14 Thread Michael D Kinney 
https://bugzilla.tianocore.org/show_bug.cgi?id=767

The USB I/O Protocol function ControlTransfer() has a DataLength
parameter that specifies the size of the Data buffer.  The
UsbBusDxe module implements the USB I/O Protocol using the
services of the USB2 Host Controller Protocol.  The DataLength
parameter in the USB2 Host Controller Protocol ControlTransfer()
service is an IN OUT parameter so the number of bytes actually
transferred is returned.  Since the USB I/O Protocol
ControlTransfer() service can not return the number of bytes
actually transferred, the only option if the number of bytes
actually transferred is less than the number of bytes requested
is to return EFI_DEVICE_ERROR.

The change fixes an issue with a USB mass storage device that
responds with 0 bytes to the Get MAX LUN command.

Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c 
b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
index 78220222f6..b0ad7938e9 100644
--- a/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
+++ b/MdeModulePkg/Bus/Usb/UsbBusDxe/UsbBus.c
@@ -2,7 +2,7 @@
 
 Usb Bus Driver Binding and Bus IO Protocol.
 
-Copyright (c) 2004 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2004 - 2017, Intel Corporation. All rights reserved.
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD 
License
 which accompanies this distribution.  The full text of the license may be 
found at
@@ -76,6 +76,7 @@ UsbIoControlTransfer (
   USB_ENDPOINT_DESC   *EpDesc;
   EFI_TPL OldTpl;
   EFI_STATUS  Status;
+  UINTN   RequestedDataLength;
 
   if (UsbStatus == NULL) {
 return EFI_INVALID_PARAMETER;
@@ -86,6 +87,7 @@ UsbIoControlTransfer (
   UsbIf  = USB_INTERFACE_FROM_USBIO (This);
   Dev= UsbIf->Device;
 
+  RequestedDataLength = DataLength;
   Status = UsbHcControlTransfer (
  Dev->Bus,
  Dev->Address,
@@ -99,6 +101,10 @@ UsbIoControlTransfer (
  >Translator,
  UsbStatus
  );
+  if (!EFI_ERROR (Status) && DataLength < RequestedDataLength) {
+Status = EFI_DEVICE_ERROR;
+goto ON_EXIT;
+  }
 
   if (EFI_ERROR (Status) || (*UsbStatus != EFI_USB_NOERROR)) {
 //
-- 
2.14.2.windows.3

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel