On Fri, 07 Jul, 2023 10:40:35 +0000 Fabio Baltieri <fabiobaltieri@xxxxxxxxxxxx> wrote:
> Add a hid-stadiaff module to support rumble based force feedback on the
> Google Stadia controller. This works using the HID output endpoint
> exposed on both the USB and BLE interface.
>
> Signed-off-by: Fabio Baltieri <fabiobaltieri@xxxxxxxxxxxx>
> ---
> +static int stadiaff_init(struct hid_device *hid)
> +{
> + struct stadiaff_device *stadiaff;
> + struct hid_report *report;
> + struct hid_input *hidinput;
> + struct input_dev *dev;
> + int error;
> +
> + if (list_empty(&hid->inputs)) {
> + hid_err(hid, "no inputs found\n");
> + return -ENODEV;
> + }
> + hidinput = list_entry(hid->inputs.next, struct hid_input, list);
> + dev = hidinput->input;
> +
> + report = hid_validate_values(hid, HID_OUTPUT_REPORT,
> + STADIA_FF_REPORT_ID, 0, 2);
> + if (!report)
> + return -ENODEV;
> +
> + stadiaff = devm_kzalloc(&hid->dev, sizeof(struct stadiaff_device),
> + GFP_KERNEL);
> + if (!stadiaff)
> + return -ENOMEM;
If we fail to allocate stadiaff, we abort init without ever initializing
the spinlock and work struct.
> +
> + hid_set_drvdata(hid, stadiaff);
> +
> + input_set_capability(dev, EV_FF, FF_RUMBLE);
> +
> + error = input_ff_create_memless(dev, NULL, stadiaff_play);
> + if (error)
> + return error;
Lets say input_ff_create_memless fails. The spinlock and work struct are
not properly initialized.
> +
> + stadiaff->removed = false;
> + stadiaff->hid = hid;
> + stadiaff->report = report;
> + INIT_WORK(&stadiaff->work, stadiaff_work);
> + spin_lock_init(&stadiaff->lock);
> +
> + hid_info(hid, "Force Feedback for Google Stadia controller\n");
> +
> + return 0;
> +}
> +
> +static int stadia_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + int ret;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "parse failed\n");
> + return ret;
> + }
> +
> + ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_FF);
> + if (ret) {
> + hid_err(hdev, "hw start failed\n");
> + return ret;
> + }
> +
> + stadiaff_init(hdev);
Is the intention for not error handling stadiaff_init to be able to use
the HID device even if haptics are not enabled? I think that's fine but
there are some design considerations that need to be made in order for
this code to be safe in the context of stadia_remove.
> +
> + return 0;
> +}
> +
> +static void stadia_remove(struct hid_device *hid)
> +{
> + struct stadiaff_device *stadiaff = hid_get_drvdata(hid);
stadiaff is unsafe to use if we failed to allocate memory for it and do
not set the hid device driver data. We would be dereferencing a
driver_data pointer never set/initialized by this driver in this error
path in stadiaff_init.
> + unsigned long flags;
> +
> + spin_lock_irqsave(&stadiaff->lock, flags);
> + stadiaff->removed = true;
> + spin_unlock_irqrestore(&stadiaff->lock, flags);
Attempting to lock/unlock a spinlock that may not have been initialized
if an error occurred in the stadiaff_init path.
> +
> + cancel_work_sync(&stadiaff->work);
Attempting to cancel work on a work_struct that may not be properly
initialized if an error occurred in stadiaff_init.
> + hid_hw_stop(hid);
> +}
Thanks,
-- Rahul Rameshbabu
