From patchwork Thu Apr 17 17:50:25 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 4010061 X-Patchwork-Delegate: jikos@jikos.cz Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 625D4BFF02 for ; Thu, 17 Apr 2014 17:50:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 63F5120328 for ; Thu, 17 Apr 2014 17:50:29 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 17B2C20320 for ; Thu, 17 Apr 2014 17:50:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751207AbaDQRu1 (ORCPT ); Thu, 17 Apr 2014 13:50:27 -0400 Received: from mail-ob0-f179.google.com ([209.85.214.179]:44770 "EHLO mail-ob0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751090AbaDQRu0 (ORCPT ); Thu, 17 Apr 2014 13:50:26 -0400 Received: by mail-ob0-f179.google.com with SMTP id va2so822071obc.10 for ; Thu, 17 Apr 2014 10:50:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=XP0nifQ7Q6QCkW2ZTC+wQszEQf1SE/41zdWaI0bTsE4=; b=ciSUQca80bQ02roKyZD6pbg6ASc4YDULPXuwjJBGvYFHqOutRhE9/HmbQHhF4BZLDi c4P0N18Pdi5sMLLqgWd3F/Ao/tnP4EEu8rPlCfV84RXVfXHVFLG3xk1JwkL1/8kHNUJG xMh9wZc1ssK2DIDgQP92Mq/TkMbtv7JHvLTQR1B5DlIidaU9nqLpShbxDtcvlcSclh/D VU/dXCfMK4LRB5X82vl/vi2gnry5tg/bx2/2FW0vc+Wx3OURhDFYKZX31nbc5SxFRiBA Bs5NRkJm/rv71hneDX33r9qpXrEKmohIslMPLCe+8As3rNNKikHaYKsRveP+UAy8dki8 sxkg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=XP0nifQ7Q6QCkW2ZTC+wQszEQf1SE/41zdWaI0bTsE4=; b=SEuzmu+vfyuIYJ2OGrDOuPviiQ7KnD3mgt4GQNpQ+PBwpUA4PWrJuIJFpc7TuvRqA9 Zme5pDCR4w99mvvK1DXdQzLAv3/yfBKQCbyvn4vjVATB5g+ei6iPAycd5pMk1YGhM4nK 6mjHakJsjRRDkafMywVALZyTIUxzZR5fKfluI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=XP0nifQ7Q6QCkW2ZTC+wQszEQf1SE/41zdWaI0bTsE4=; b=gmX+1lnFeUou28jf9QsQkXdCWOd5nZm5K+M2D9euyFjhz7cLq867//kFo4I6Zt6sk2 GXiY4j12IzTCeqdQtdR2VU7ee7LuU/WQbTsevY6zBsAdSrGHpE7AuMYns5Jfoq/RvVVd ZAKzR5cH1o8/S+A2MASZJy8DG+9YpFiv3Ti16FnvkJcOb6RCwYsZiIW3cXeTJ2hJ/2eb WZUp2CsMmapa5z0Se8BPuvIE7BMQXo8ZpR5B0eyPAQi5C2kExN4q/rKnoxde2efRppjh DxhtZ5YIxsk/mQIeqmCfqphYvPercyJHjiyw4r0VflgzmLYDVjKkMc9A+2OtPk1cuQhI wKng== X-Gm-Message-State: ALoCoQn8/2JFxrLjVatRZe65FX5eKlgHKTE2wN4YcKAZfyk7zclybnN0CgBIT8lIzuHksMslJKU0WnTpujIioelUrdSedVAGnwYJbK62OtZwRPv8aGDYfoIOjuJWcVvY1pTqykCpHFItrd6qv1+UWbovW9tFYxCjcPN/TyS3NiH7MShyD4EfoGcTepEyQQkhm+0SEzJkFOQqDugsjd+CuplaGBr4TLbi+A== MIME-Version: 1.0 X-Received: by 10.60.51.227 with SMTP id n3mr13008559oeo.33.1397757025445; Thu, 17 Apr 2014 10:50:25 -0700 (PDT) Received: by 10.182.226.163 with HTTP; Thu, 17 Apr 2014 10:50:25 -0700 (PDT) In-Reply-To: <20140417173723.GC10689@mail.corp.redhat.com> References: <6452400bfacafd7e1f0d0f7a98b06248.squirrel@mungewell.org> <796aa99e55f8812aa6422d35610d4ed3.squirrel@mungewell.org> <20140417173723.GC10689@mail.corp.redhat.com> Date: Thu, 17 Apr 2014 10:50:25 -0700 X-Google-Sender-Auth: RgXob3QV4fBruttyidBKbmS5kUc Message-ID: Subject: Re: HID: hid-logitech - missing HID_OUTPUT_REPORT 0 From: Kees Cook To: Benjamin Tissoires Cc: Simon Wood , linux-input , Jiri Kosina , Elias Vanderstuyft Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Apr 17, 2014 at 10:37 AM, Benjamin Tissoires wrote: > On Apr 17 2014 or thereabouts, Kees Cook wrote: >> On Thu, Apr 17, 2014 at 9:35 AM, wrote: >> > >> >> I don't know the lg driver very well, but it looks like it's expecting >> >> a single report ID (0), but the device is showing two report IDs: 1 >> >> and 2. So, from the perspective of the driver, this is correct: it >> >> wouldn't know how to deal with things since it is only expecting >> >> Report ID 0. It seems like the driver needs to be updated for this >> >> different device. >> > >> > Hi, >> > The 'hid-lgff.c' driver supports lots of devices (see end of 'hid-lg.c'), >> > and presumably these devices offer up a wide/varied range of HID >> > descriptors. >> > >> > Does the recently introduced(/changed) check need to have prior knowledge >> > of which 'Report ID's are actually used? If so, it possible that the >> > change has broken a number of devices... >> > >> > I am trying to get the end user to test with an older kernel to see >> > whether his device was always 'broken'. >> >> Ah-ha, actually, when taking a closer look at this, I see that lgff >> isn't using ID "0", it's actually using "the first report in the >> list", without using an ID at all. This appears to be true for all the >> lg*ff devices, actually. Instead of validating ID 0, it needs to >> validate "the first report". >> >> Documentation for hid_validate_values says: >> * @id: which report ID to examine (0 for first) >> >> Benjamin, does that mean "first report in the list", or is that a hint > > yep > >> that IDs are 0-indexed? > > nope :) > > page 46 of the HID 1.11 spec (http://www.usb.org/developers/devclass_docs/HID1_11.pdf) > says: "Report ID zero is reserved and should not be used." Ah! Perfect, yes. And I see we're doing that validation: case HID_GLOBAL_ITEM_TAG_REPORT_ID: parser->global.report_id = item_udata(item); if (parser->global.report_id == 0 || parser->global.report_id >= HID_MAX_IDS) { hid_err(parser->device, "report_id %u is invalid\n", parser->global.report_id); return -1; } return 0; >> What do you think is the best way to handle >> this? Seems like passing something for "id" that means "whatever is >> first in list" would be safest? I don't think overloading the meaning >> of "0" is good, in case a driver really is using a 0 index but no >> report with that ID exists in the list. > > "Overloading" 0 here is fine because reportID==0 can not exist according > to the spec. Actually, a HID device is not forced to use report IDs at > all if it sends only one type of reports. > So in the various transport layers, 0 is handled as a special case > anyway, and that means that there is no report ID. And when there is no > report ID, there should be only one which is the first in the list :) > > Still, hid-lg should not use this trick to find the first report, but > this driver has quite a lot of history, so I will not try to fix it. > > Anyway, it looks like hid_validate_values has to be fixed to handle HID > devices without a report ID (which would fix hid-lg too). > >> Or if we do change the >> meaning, make sure drivers always use the report returned by >> hid_validate_values instead of re-finding it later. > > As explained above, it shouldn't matter. And it's more likely a bug in > hid_validate_values that I should have spot when reviewing it :/ How does this look? (Likely whitespace damaged -- I can resend correctly if it's what you had in mind.) -Kees diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 9e8064205bc7..5205ebb76cd2 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -840,6 +840,15 @@ struct hid_report *hid_validate_values(struct hid_device *h * drivers go to access report values. */ report = hid->report_enum[type].report_id_hash[id]; + if (!report && id == 0) { + /* + * Requesting id 0 means we should fall back to the first + * report in the list. + */ + report = list_entry( + hid->report_enum[type].report_list.next, + struct hid_report, list); + } if (!report) { hid_err(hid, "missing %s %u\n", hid_report_names[type], id); return NULL; Alternatively, should hid_register_report add a default to the hash instead, so no change to hid_validate_values is needed? diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 9e8064205bc7..5d07124457ba 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -80,6 +80,8 @@ struct hid_report *hid_register_report(struct hid_device *devi report->size = 0; report->device = device; report_enum->report_id_hash[id] = report; + if (!report_enum->report_id_hash[0]) + report_enum->report_id_hash[0] = report; list_add_tail(&report->list, &report_enum->report_list);