Re: corsair-cpro and hidraw

[Wilken Gottwalt <wilken.gottwalt@posteo.net>]

On Fri, 18 Jun 2021 05:56:29 +0600
Aleksandr Mezin <mezin.alexander@gmail.com> wrote:

> I've looked through corsair-psu sources, and I think filtering in
> raw_event won't be enough.
> 
> For example, in corsairpsu_request, there are 2 commands, and they
> should be executed consecutively:
> 1) corsairpsu_usb_cmd(priv, 2, PSU_CMD_SELECT_RAIL, rail, NULL);
> 2) corsairpsu_usb_cmd(priv, 3, cmd, 0, data);
> 
> If the userspace will squeeze another PSU_CMD_SELECT_RAIL between (1)
> and (2), the driver will get data for a wrong rail (and with the
> current code won't even notice it).
> 
> So unless there is a way to "lock" hidraw (and it seems that there
> isn't - looking at the code, hidraw calls the low-level hid driver
> directly, as far as I understand), it won't work correctly.
> 
> And if a driver can't work correctly with hidraw enabled - maybe it
> shouldn't enable hidraw? So that the user can 1) notice that something
> is wrong 2) blacklist or unbind the driver (or userspace tools that
> use hidraw can unbind automatically). Otherwise everything seems to be
> silently broken.
> 
> On the other hand, maybe races between the kernel driver and userspace
> tools are unlikely, because the driver doesn't talk to the device
> continuously - only when sysfs reads happen.

I never noticed any issues of that kind. I actually did quite a lot of
userspace testing. A result of this a userspace tool you can find here:
https://github.com/wgottwalt/corsair-psu/tree/main/tools/rmi-hxi-query

Though, if you find a way to trigger such a race condition I have no
problem to remove the hidraw part.

greetings
Will

> Added corsair-psu maintainer to Cc:
> 
> On Thu, Jun 17, 2021 at 7:14 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Thu, Jun 17, 2021 at 01:11:38PM +0600, Aleksandr Mezin wrote:
> > > On Thu, Jun 17, 2021 at 12:27 PM Marius Zachmann <mail@mariuszachmann.de> wrote:
> > > ...
> > > > I do not know, what your device is doing
> > >
> > > Actually, NZXT devices (at least grid/"smart device" and "smart device
> > > v2"/rgb&fan controller) don't have such issues - they use report ids,
> > > and don't even expect request-reply communication pattern. I've just
> > > noticed that something seems to be wrong with corsair-cpro (but
> > > somehow didn't notice the comment) and decided to ask.
> > >
> > > > This device uses an echo of the command
> > > > in the answer and if they don't match it returns an error. This could
> > > > maybe lead to a false error when the replies are switched, but is
> > > > probably preferable.
> > >
> > > Hm... If the response includes the id of the request, it should be
> > > possible to filter reports in raw_event, i. e. don't signal completion
> > > if the report doesn't match, and wait more. Yes, there is a corner
> > > case, "if a command is not supported, the length value in the reply is
> > > okay, but the command value is set to 0". But timing out (250 ms) in
> > > this case should probably be fine... Actually I have a compatible
> > > Corsair PSU so maybe I'll send a patch.
> >
> > Patches to improve the situation are welcome. My understanding is
> > that with the current driver users should disable the kernel driver
> > if they plan to use userspace tools to access the device.
> >
> > Guenter