Re: [PATCH 08/11] MdeModulePkg/AbsPointer: 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 at least 3-byte
> long.
> Today's code assumes that the key codes buffer length is longer than
> 3-byte 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 3-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>
> ---
>   .../Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c  | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
> index 965195ca34..b4638961d9 100644
> --- a/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
> +++ b/MdeModulePkg/Bus/Usb/UsbMouseAbsolutePointerDxe/UsbMouseAbsolutePointer.c
> @@ -813,8 +813,6 @@ OnMouseInterruptComplete (
>       return EFI_SUCCESS;
>     }
>   
> -  UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
> -
>     //
>     // Check mouse Data
>     // USB HID Specification specifies following data format:
> @@ -827,6 +825,12 @@ OnMouseInterruptComplete (
>     // 2       0 to 7  Y displacement
>     // 3 to n  0 to 7  Device specific (optional)
>     //
> +  if ((Data != NULL) && (DataLength < 3)) {
> +    return EFI_DEVICE_ERROR;
> +  }

Ray,

Thanks for the patch.
Data is impossible to be NULL here as 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;
    }

Thanks,
Star

> +
> +  UsbMouseAbsolutePointerDevice->StateChanged = TRUE;
> +
>     UsbMouseAbsolutePointerDevice->State.ActiveButtons = *(UINT8 *) Data & (BIT0 | BIT1 | BIT2);
>   
>     UsbMouseAbsolutePointerDevice->State.CurrentX =
>