Re: [PATCH] HID: validate report length and constants

[Davide Beatrici <me@davidebeatrici.dev>]

> However, the device also shows an output report of size 1, but it is 
> not
> supposed to send it as an input report. I wonder if the firmware bug is
> not that it tries to give the host the current state of its output
> report at plug (which is wrong but Windows must be papering over it).

On that note, I noticed the malformed packet is not sent upon 
reconnecting the
device if it's been plugged in for some time.
When that happens I can reproduce the issue by enabling a wireless mode 
through
the switch on the bottom and wait a bit before disabling it and 
connecting the
device again.
I suspect the battery gets slightly discharged and the device sends a
"charging complete" signal or something.

> - the URB is of size 1, so the fact that the constant field is not 0
> means that we are just reading random memory at offset 1 in the
> provided data, so you might have a chance that it eventually becomes 0

Good point, if it's effectively random memory we cannot rely on that.

> - the fix should be focusing on the length of the provided report, not
> on the content. However, in hid_report_raw_event(), just before you
> inserted your call to your hid_validate_report(), there is already a
> check on the length of the report which memsets to 0 the rest of the
> buffer. This seems a little bit optimistic if the provided buffer from
> USB is exactly the size of the provided "size" argument.
> But then, why would you get random data in the const fields if there
> is a memset if the provided length is "1"?
> 
> So, can you add a printk before your call to hid_validate_report() to
> show the provided "size" argument (csize), or just enable the hid_dbg()
> trace output which should tell us if we enter that test and do the
> memset (which I suppose we are not).

report 8 has csize=16 rsize=16
report 0 has csize=1 rsize=8
report 0 is too short, (1 < 8)

Which means we do enter the test and execute the memset()...