From patchwork Tue Jan 19 06:40:37 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Torokhov X-Patchwork-Id: 8058391 Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id E7D829F96D for ; Tue, 19 Jan 2016 06:41:16 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0AE1D203B1 for ; Tue, 19 Jan 2016 06:41:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 07C2C20411 for ; Tue, 19 Jan 2016 06:41:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751146AbcASGkn (ORCPT ); Tue, 19 Jan 2016 01:40:43 -0500 Received: from mail-pa0-f54.google.com ([209.85.220.54]:33340 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751148AbcASGkk (ORCPT ); Tue, 19 Jan 2016 01:40:40 -0500 Received: by mail-pa0-f54.google.com with SMTP id cy9so446410754pac.0 for ; Mon, 18 Jan 2016 22:40:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:mime-version :content-type:content-disposition:user-agent; bh=44vQ2SvS/lDg76u7TTBCNqS1oi4kfZ0QbBUBSazD8NQ=; b=HeydyUELetCGDUcSLXvVDGMIJMIVHliVyEuZdwO3DObT99ClQtV/j3ojXNiKC2Okg/ h4EhM3MdvjtAsy5jnaEzUnVNwYTSQOAP8i0W69+4NdNu9VkkYD/E5Gp9WW+OFucckU6L 88jiSbNwnt48ebmHNB3LK+QpIAFlb1s4GGTxy7BBcRs9b3p99YfWpJChH/6WDH1XuhaD niIY18jAQZbYtprdavcpM3vKD9qgq06Wzb7PFioK62kZMnQjU1kYpf2djFAKidqtSHvf hVJc0ZUqx577if3KCj/uaV78gJlDVsDDLSMpbjEYVPYaaNJFp1/OMxwD27ghwOjdc3fC nDBA== X-Gm-Message-State: ALoCoQn6d1+Cl7/L/u8j54/PV15jOiyyKrU+sCTk8nUeWtBD7DOBo2ZpACb4Zhtc/Y5LZmY9Pr0DAQKDuCzcYzcJ5eQSO3A8cw== X-Received: by 10.66.142.73 with SMTP id ru9mr41783859pab.121.1453185640205; Mon, 18 Jan 2016 22:40:40 -0800 (PST) Received: from dtor-ws ([2620:0:1000:1301:6006:c84c:9e69:1564]) by smtp.gmail.com with ESMTPSA id yl1sm38626092pac.35.2016.01.18.22.40.39 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Mon, 18 Jan 2016 22:40:39 -0800 (PST) Date: Mon, 18 Jan 2016 22:40:37 -0800 From: Dmitry Torokhov To: Jiri Kosina Cc: Benjamin Tissoires , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH] HID: fix out of bound access in extract and implement Message-ID: <20160119064037.GA18052@dtor-ws> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable 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 extract() and implement() access buffer containing reports in 64-bit chunks, but there is no guarantee that buffers are padded to 64 bit boundary. In fact, KASAN has caught such OOB access with i2c-hid and Synaptics touch controller. Instead of trying to hunt all parties that allocate buffers and make sure they are padded, let's switch extract() and implement() to byte access. It is a bit slower, bit we are not dealing with super fast devices here. Also let's fix link to the HID spec while we are at it. Signed-off-by: Dmitry Torokhov Reviewed-by: Benjamin Tissoires --- drivers/hid/hid-core.c | 90 ++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 66 insertions(+), 24 deletions(-) diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 9e182e4..9f1019d 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -1075,7 +1075,7 @@ static u32 s32ton(__s32 value, unsigned n) * Extract/implement a data field from/to a little endian report (bit array). * * Code sort-of follows HID spec: - * http://www.usb.org/developers/devclass_docs/HID1_11.pdf + * http://www.usb.org/developers/hidpage/HID1_11.pdf * * While the USB HID spec allows unlimited length bit fields in "report * descriptors", most devices never use more than 16 bits. @@ -1083,20 +1083,37 @@ static u32 s32ton(__s32 value, unsigned n) * Search linux-kernel and linux-usb-devel archives for "hid-core extract". */ -__u32 hid_field_extract(const struct hid_device *hid, __u8 *report, - unsigned offset, unsigned n) -{ - u64 x; +static u32 __extract(u8 *report, unsigned offset, int n) +{ + unsigned int idx = offset / 8; + unsigned int bit_nr = 0; + unsigned int bit_shift = offset % 8; + int bits_to_copy = 8 - bit_shift; + u32 value = 0; + u32 mask = n < 32 ? (1U << n) - 1 : ~0U; + + while (n > 0) { + value |= ((u32)report[idx] >> bit_shift) << bit_nr; + n -= bits_to_copy; + bit_nr += bits_to_copy; + bits_to_copy = 8; + bit_shift = 0; + idx++; + } + + return value & mask; +} - if (n > 32) +u32 hid_field_extract(const struct hid_device *hid, u8 *report, + unsigned offset, unsigned n) +{ + if (n > 32) { hid_warn(hid, "hid_field_extract() called with n (%d) > 32! (%s)\n", n, current->comm); + n = 32; + } - report += offset >> 3; /* adjust byte index */ - offset &= 7; /* now only need bit offset into one byte */ - x = get_unaligned_le64(report); - x = (x >> offset) & ((1ULL << n) - 1); /* extract bit field */ - return (u32) x; + return __extract(report, offset, n); } EXPORT_SYMBOL_GPL(hid_field_extract); @@ -1106,31 +1123,56 @@ EXPORT_SYMBOL_GPL(hid_field_extract); * The data mangled in the bit stream remains in little endian * order the whole time. It make more sense to talk about * endianness of register values by considering a register - * a "cached" copy of the little endiad bit stream. + * a "cached" copy of the little endian bit stream. */ -static void implement(const struct hid_device *hid, __u8 *report, - unsigned offset, unsigned n, __u32 value) + +static void __implement(u8 *report, unsigned offset, int n, u32 value) +{ + unsigned int idx = offset / 8; + unsigned int size = offset + n; + unsigned int bit_shift = offset % 8; + int bits_to_set = 8 - bit_shift; + u8 bit_mask = 0xff << bit_shift; + + while (n - bits_to_set >= 0) { + report[idx] &= ~bit_mask; + report[idx] |= value << bit_shift; + value >>= bits_to_set; + n -= bits_to_set; + bits_to_set = 8; + bit_mask = 0xff; + bit_shift = 0; + idx++; + } + + /* last nibble */ + if (n) { + if (size % 8) + bit_mask &= (1U << (size % 8)) - 1; + report[idx] &= ~bit_mask; + report[idx] |= (value << bit_shift) & bit_mask; + } +} + +static void implement(const struct hid_device *hid, u8 *report, + unsigned offset, unsigned n, u32 value) { - u64 x; - u64 m = (1ULL << n) - 1; + u64 m; - if (n > 32) + if (n > 32) { hid_warn(hid, "%s() called with n (%d) > 32! (%s)\n", __func__, n, current->comm); + n = 32; + } + m = (1ULL << n) - 1; if (value > m) hid_warn(hid, "%s() called with too large value %d! (%s)\n", __func__, value, current->comm); WARN_ON(value > m); value &= m; - report += offset >> 3; - offset &= 7; - - x = get_unaligned_le64(report); - x &= ~(m << offset); - x |= ((u64)value) << offset; - put_unaligned_le64(x, report); + __implement(report, offset, n, value); } /*