Re: [PATCH] HID: logitech-hidpp: Enable HID++ for all the Logitech Bluetooth devices

["Peter F. Patel-Schneider" <pfpschneider@gmail.com>]

The bad interaction caused by the bug goes like this.

The driver tries to get the battery information and finds out that the HID++ 
2.0 device has the Unified Battery feature (0x1004) at index 0x08.  It sends a 
command to the device to report the capabilities its Unified Battery feature 
supports using

#define CMD_UNIFIED_BATTERY_GET_CAPABILITIES            0x00

for the command and software identifier nibbles in byte 4 of the message, 
which ends up being

11 FF 0800 00000000000000000000000000000000

This is a non-conforming message because the software identifier nibble (the 
low-order nibble of byte 4) is 0x0.

The device does not respond with an error indicating that the message is 
non-conforming but instead treats the message as conformant and responds with 
the capabilities of its Unified Battery feature

11 FF 0800 0F030000000000000000000000000000

copying the device number, feature index, function, and software identifier, 
as required.  The rest of the response indicates that the feature on the 
device supports all four qualitative battery levels, is rechargeable, and also 
reports battery levels in percentages.

Devices with this feature also emit notification messages when their batteries 
change level.  Notification messages are distinguished from response messages 
by having a 0x0 software identification nibble.  So this response is read by 
other software that is listening to the raw HID messages from the device and 
interpreted as an event notification.

In this case the feature has an event notification with function 0x0 that is 
emitted when status of the device's battery is changed.  The fifth byte of 
these notification messages is the percentage of battery energy available.  So 
the software concludes that the device's battery has 15% of its energy remaining.


If the driver is changed to

#define CMD_UNIFIED_BATTERY_GET_CAPABILITIES            0x01

then the command sent would be

11 FF 0801 00000000000000000000000000000000

which is conforming, and the response would be

11 FF 0801 0F030000000000000000000000000000

which is not the same as any event notification message.



Other bad interactions are also possible, including the the driver 
misinterpreting an event notification as a response to a message that it sent 
to the device.


peter



On 8/26/22 10:35, Bastien Nocera wrote:
> On Thu, 2022-08-25 at 16:47 -0400, Peter F. Patel-Schneider wrote:
>> This patch will cause more use of a part of the driver that
>> constructs
>> messages that do not conform to the HID++ 2.0 specification.  This
>> makes now a good time to fix the parts of the driver that construct
>> non-conforming messages.  More information follows.
> This would cause problems, but not any worse than adding the product
> IDs individually, which is what we're trying to avoid.
>
>> This results in non-conforming messages being sent to devices.  As
>> devices are obligated to return this nibble intact they produce non-
>> conforming responses as well.  (Their other option would be to reject
>> the messages.) This confuses other software that correctly uses this
>> nibble to distinguish between device response messages and device
>> event
>> messages.
> I don't understand how this...
>
>> In particular, the response to the unified battery command to get the
>> capabilities comes back with a 0x00 function byte which is
>> indistinguishable from a spontaneous notification message from the
>> device for a battery status event.  Other software trying to
>> communicate with the device (e.g., Solaar) sees a unified battery
>> status notification and will generally end up with incorrect
>> information about the device.  I suspect that this is actually
>> happening and is the cause of the Solaar bug report
>> https://github.com/pwr-Solaar/Solaar/issues/1718
> ...could cause this. Can you explain what the messages would look like
> in both cases, and how they could be misinterpreted as a 15 vs. 85
> percent battery level?
>
>> There is also the possibility that the driver confuses a notification
>> from the device as the response to a command that it sent.  When this
>> happens it would be likely that the actual response would be treated
>> as
>> a notification.
>>
>>
>> The fix is to modify all the CMD definitions in the code to have 1 in
>> their low-order nibble.
> All in all, I don't see those bugs as blocking the integration of the
> patch discussed above, I see it as a way to expose those bugs and
> possibly a way to make them more urgent.
>
> Filipe, were those problems known/already reported?
>
> Cheers


I reported https://bugzilla.kernel.org/show_bug.cgi?id=215699 which is a 
different problem with the same cause, albeit in a different constant in the 
driver.


peter