From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 20B4BEB64D9 for ; Fri, 7 Jul 2023 21:35:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231962AbjGGVfN (ORCPT ); Fri, 7 Jul 2023 17:35:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37680 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229940AbjGGVfM (ORCPT ); Fri, 7 Jul 2023 17:35:12 -0400 Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0EA8D1FD8 for ; Fri, 7 Jul 2023 14:35:11 -0700 (PDT) Received: by mail-ej1-x635.google.com with SMTP id a640c23a62f3a-9924ac01f98so305546266b.1 for ; Fri, 07 Jul 2023 14:35:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1688765709; x=1691357709; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=EreKLP61fJFEZifwXUgdWDfgup62n62CRKHFNhat6Mg=; b=e+fylLZ8AKeITK916W6p8mq1bmTwXBJYnAptPo3i6gOOrFhG/JqgrfWJFmcXfehF3T cgfCtRxiH5untqQDl4tcHKc62ERZyC0rEYmN/5XropK6wEkQ+vC61jIsHfcDXuZEauoy umbbJ9lsOdhLYlI73Kxq5mAGy514otKg9t5XU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1688765709; x=1691357709; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=EreKLP61fJFEZifwXUgdWDfgup62n62CRKHFNhat6Mg=; b=Ftj5Una7UCBVlkERPMfB8z519/aQpPoS9PNtZY0Wx6FAKt5kASV1flzm2G06pbRBhY BO9kNRE4YrIpbCK+Sh8wlu6UkKqmP8eit8h0vhV6yZ3FRRvqc6e89uAm8cGQmT5Dig3A 0qQEO0e3OJrL+OqFxjDHXH7SwSzDdqGf6IzcQi7F/RaNHWAGdzaKY14KLcb4PzcJyZBW crzlmJ3ZT0da8MgBYTzeZmECpR90n14wRnueFE97zdMU9AVDZHJPFwGIm910SsEOG/zi VwoqGo0CcU1toWfBJ3CelyveY1ag8bzer1NJuMazlju9G5YB7KlAV22bGPYYgqLa81LU dkdw== X-Gm-Message-State: ABy/qLbzRam9dYIN4YT96tZNHJkqF8WG2q5Uyd8bCfEYH0fa9Smh9cKM izI5ZCywYwk30G1shra/X63rHQ4QV9yahKHKCYJ6AA== X-Google-Smtp-Source: APBJJlF2514EjcgkLq4ojDadFsNSQboIYDjQfkCJeC1TaSLwGWfPChIEGA1mM5mMw/p77wQ0wNByjA== X-Received: by 2002:a17:907:58d:b0:991:e815:a1ef with SMTP id vw13-20020a170907058d00b00991e815a1efmr4157392ejb.31.1688765709396; Fri, 07 Jul 2023 14:35:09 -0700 (PDT) Received: from google.com (64.227.90.34.bc.googleusercontent.com. [34.90.227.64]) by smtp.gmail.com with ESMTPSA id o13-20020a170906358d00b0098d15d170a0sm2605738ejb.202.2023.07.07.14.35.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Jul 2023 14:35:09 -0700 (PDT) Date: Fri, 7 Jul 2023 21:35:07 +0000 From: Fabio Baltieri To: Rahul Rameshbabu Cc: Benjamin Tissoires , Jiri Kosina , 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: 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 List-ID: X-Mailing-List: linux-input@vger.kernel.org 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 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 > > --- > > +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