Re: [PATCH v2 RESEND] HID: hid-google-stadiaff: add support for Stadia force feedback

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

On Fri, 07 Jul, 2023 10:40:35 +0000 Fabio Baltieri <fabiobaltieri@chromium.org> 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@chromium.org>
> ---
> +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