[bug report] HID: ft260: add usb hid to i2c host bridge driver

Hello Michael Zaidman,

The patch 6a82582d9fa4: "HID: ft260: add usb hid to i2c host bridge
driver" from Feb 19, 2021, leads to the following static checker
warning:

	drivers/hid/hid-ft260.c:1028 ft260_raw_event()
	error: 'xfer->length' from user is not capped properly

drivers/hid/hid-ft260.c
  1017  static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
  1018                             u8 *data, int size)
  1019  {
  1020          struct ft260_device *dev = hid_get_drvdata(hdev);
  1021          struct ft260_i2c_input_report *xfer = (void *)data;
  1022  
  1023          if (xfer->report >= FT260_I2C_REPORT_MIN &&
  1024              xfer->report <= FT260_I2C_REPORT_MAX) {
  1025                  ft260_dbg("i2c resp: rep %#02x len %d\n", xfer->report,
  1026                            xfer->length);
  1027  
  1028                  memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
  1029                         xfer->length);

Do we need to check if "xfer->len <= dev->read_len"?

  1030                  dev->read_idx += xfer->length;
  1031  
  1032                  if (dev->read_idx == dev->read_len)
  1033                          complete(&dev->wait);
  1034  
  1035          } else {
  1036                  hid_err(hdev, "unknown report: %#02x\n", xfer->report);
  1037                  return 0;
  1038          }
  1039          return 1;
  1040  }

regards,
dan carpenter

On Thu, Mar 18, 2021 at 01:39:53PM +0300, Dan Carpenter wrote:

drivers/hid/hid-ft260.c
  1017  static int ft260_raw_event(struct hid_device *hdev, struct hid_report *report,
  1018                             u8 *data, int size)
  1019  {
  1020          struct ft260_device *dev = hid_get_drvdata(hdev);
  1021          struct ft260_i2c_input_report *xfer = (void *)data;
  1022  
  1023          if (xfer->report >= FT260_I2C_REPORT_MIN &&
  1024              xfer->report <= FT260_I2C_REPORT_MAX) {
  1025                  ft260_dbg("i2c resp: rep %#02x len %d\n", xfer->report,
  1026                            xfer->length);
  1027  
  1028                  memcpy(&dev->read_buf[dev->read_idx], &xfer->data,
  1029                         xfer->length);

Do we need to check if "xfer->len <= dev->read_len"?

The dev->read_len is a total length to be read, passed into ft260_i2c_read()
by a user. This length is divided into 60 bytes chanks to be retrieved from
the ft260 controller. The ft260_raw_event() receives these chanks and counts
the total number of bytes received in read_idx. Once it matches the read_len,
we conclude on the i2c read transfer completion. We do not need to check the
xfer->len against the dev->read_len since the ft260 controller never returns
more bytes than was asked to read.

  1030                  dev->read_idx += xfer->length;
  1031  
  1032                  if (dev->read_idx == dev->read_len)
  1033                          complete(&dev->wait);
  1034  
  1035          } else {
  1036                  hid_err(hdev, "unknown report: %#02x\n", xfer->report);
  1037                  return 0;
  1038          }
  1039          return 1;
  1040  }

regards,
dan carpenter