Re: [PATCH 07/11] MdeModulePkg/UsbKb: Don't access key codes when length is wrong

["Zeng, Star" <star.zeng@intel.com>]

On 2018/10/15 14:38, Ruiyu Ni wrote:
> Per USB HID spec, the buffer holding key codes should be 8-byte
> long.
> Today's code assumes that the key codes buffer length is 8-byte
> long and unconditionally accesses the key codes buffer.
> It's incorrect.
> The patch fixes the issue by returning Device Error when the
> length is less than 8-byte.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Steven Shi <steven.shi@intel.com>
> ---
>   MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> index 9cb4b5db6b..384d7e2f07 100644
> --- a/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> +++ b/MdeModulePkg/Bus/Usb/UsbKbDxe/KeyBoard.c
> @@ -1059,6 +1059,10 @@ KeyboardHandler (
>     // Byte 1 is reserved.
>     // Bytes 2 to 7 are keycodes.
>     //
> +  if ((Data != NULL) && (DataLength < 8)) {
> +    return EFI_DEVICE_ERROR;
> +  }

Ray,

Thanks for the patch.
The NULL check to Data has been done by code piece

   //
   // If no error and no data, just return EFI_SUCCESS.
   //
   if (DataLength == 0 || Data == NULL) {
     return EFI_SUCCESS;
   }

And do you think whether the code can use DataLength != 8 instead of 
DataLength < 8?

Thanks,
Star

> +
>     CurKeyCodeBuffer  = (UINT8 *) Data;
>     OldKeyCodeBuffer  = UsbKeyboardDevice->LastKeyCodeArray;
>   
>