Re: [PATCH v4 0/5] Add OneXPlayer Configuration HID Driver

["Günther Noack" <gnoack@google.com>]

Hello Derek!

On Sat, Apr 18, 2026 at 09:26:19PM -0700, Derek J. Clark wrote:
> Adds an HID driver for OneXPlayer HID configuration devices. There are
> currently 2 generations of OneXPlayer HID protocol. The first (OneXPlayer
> F1 series) only provides an RGB control interface over HID. The Second
> (X1 mini series, G1 series, AOKZOE A1X) also includes a hardware level
> button mapping interface, vibration intensity settings, and the ability
> to switch output between xinput and a debug mode that can be used to debug
> the button mapping. Some devices (G1 Series, APEX) use a hybrid of Gen1
> RGB control and Gen 2 controller settings. To ensure there is no conflicts
> when the driver is loaded, we skip creating the RGB interface for Gen 2
> devices if there is a DMI match.
> 
> I'll also add a note that Gen 1 devices also have an interface for
> setting the key map and debug mode, but that is done entirely over a
> serial TTY device so it is not able to be added to this driver. There
> are also some "Gen 0" devices (OneXPlayer 2 Series) also use it, but
> the TTY interface also handles the RGB control so no support is
> provided by this driver for those interfaces.
> 
> Signed-off-by: Derel J. Clark <derekjohn.clark@gmail.com>

Sorry I am late to this review, but here are two issues I discovered
when looking at the code:

(1) The functions oxp_hid_raw_event_gen_1() and
    oxp_hid_raw_event_gen_2() are both forgetting to do bounds checks
    against the "size" argument.

    For real devices, which send a real report descriptor, these buffers
    will be large enough, but a device that sends a faked report
    descriptor can provoke an out-of-bounds-read here by underspecifying
    the size for these reports.

(2) oxp_hid_probe() and other functions are populating drvdata, and
    drvdata is a static variable.  If you plug in two of these devices
    at the same time, they will step on each other's toes, and this
    leads to all kinds of memory corruption problems when they do.

    I believe the right way to go about this is to allocate a separate
    piece of memory for each device that you are plugging in.  Other
    device drivers do this uing devm_kzalloc().

Disclaimer:

I found these through code inspection and curiosity but have not tried
to reproduce the crashes.

Per Linux's official threat model[1], these are not considered security
vulnerabilities.  An attacker who impersonates a USB device and gains
illegitimate access to the USB port might be able to provoke these bugs
though, and I wouldn't be surprised if (2) also just leads to system
crashes when using two of these devices at the same time.

—Günther

[1] https://docs.kernel.org/process/threat-model.html