Re: [PATCH] HID: vivaldi: Validate Report ID and length in feature mapping

[Shashikirana H K]

Hi Dmitry,

Thank you for reviewing the patch!

> Devices typically do not return short reports. Could you please tell me
> what prompted this change?

This came up while testing with synthetic/uhid devices that return
malformed GET_REPORT responses (either fewer bytes than report_len or a
different Report ID in report_data[0] than requested). When that happens,
vivaldi_feature_mapping() still passes the buffer to hid_report_raw_event(),
which then looks up a different report via data[0] or processes unreceived
bytes from the buffer.

Since legitimate Vivaldi keyboards always return the full feature report,
rejecting short reads outright with `if (ret != report_len)` (similar to
hammer_get_folded_state() in hid-google-hammer.c) would be even cleaner
than passing `ret` to hid_report_raw_event().

> I do not think checking whether report is numbered is needed. While
> transports handle numbered and non-numbered reports differently here I
> believe we always have report number (0 for unnumbered) in first byte.
> Also, shouldn't this check be in hid core if it is needed?

Agreed on both points:
1. Dropping `report->id &&` and simply checking `report_data[0] != report->id`
   (like mt_get_feature() in hid-multitouch.c) works for both numbered and
   unnumbered reports since transports place 0 in byte 0 for unnumbered
   reports.
2. Checking `buf[0] != reportnum` on HID_REQ_GET_REPORT directly in hid-core
   (e.g. in __hid_hw_raw_request()) would protect all HID drivers at once.

Jiri, Benjamin, Dmitry -- would you prefer a v2 that checks
`ret != report_len` and `report_data[0] != report->id` in
vivaldi_feature_mapping(), or moving the `buf[0] != reportnum` check for
HID_REQ_GET_REPORT into hid-core?

Thanks,
Shashikirana