Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Jul 2023 21:35:07 +0000
From:   Fabio Baltieri <fabiobaltieri@chromium.org>
To:     Rahul Rameshbabu <rrameshbabu@nvidia.com>
Cc:     Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Jiri Kosina <jikos@kernel.org>, linux-input@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 RESEND] HID: hid-google-stadiaff: add support for
 Stadia force feedback
Message-ID: <ZKiFC1Llz8VFxrDR@google.com>
References: <20230707104035.1697204-1-fabiobaltieri@chromium.org>
 <87fs5zboej.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87fs5zboej.fsf@nvidia.com>
Precedence: bulk

Hi Rahul,

On Fri, Jul 07, 2023 at 10:51:48AM -0700, Rahul Rameshbabu wrote:
> 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.

Sorry, no, the intention was to catch the error here and fail the probe,
that's the pattern on other hid haptic drivers as well, would not really
want to get too creative about it here. I shuffled the code around a bit
and somehow missed this check, I think it addresses all the potential
unallocated dereferecing you pointed out.

I'll send a v3 with the check, thanks for spotting this.

> 
> > +
> > +	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