Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sabyrzhan Tasbolatov <snovitoll@gmail.com>
To:     stern@rowland.harvard.edu
Cc:     benjamin.tissoires@redhat.com, jikos@kernel.org,
        linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org, snovitoll@gmail.com,
        syzbot+ab02336a647181a886a6@syzkaller.appspotmail.com
Subject: Re: [PATCH] drivers/hid: fix for the big hid report length
Date:   Fri, 26 Feb 2021 14:13:36 +0600
Message-Id: <20210226081336.3475085-1-snovitoll@gmail.com>
In-Reply-To: <20210225155914.GA1350993@rowland.harvard.edu>
References: <20210225155914.GA1350993@rowland.harvard.edu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 25 Feb 2021 10:59:14 -0500, Alan Stern wrote:
> Won't this cause silent errors?

Agree. But there are already such as cases like in:

// net/bluetooth/hidp/core.c
static void hidp_process_report(..)
{
	..
	if (len > HID_MAX_BUFFER_SIZE)
		len = HID_MAX_BUFFER_SIZE;
	..

// drivers/hid/hid-core.c
int hid_report_raw_event(..)
{
	..
	rsize = hid_compute_report_size(report);

	if (report_enum->numbered && rsize >= HID_MAX_BUFFER_SIZE)
		rsize = HID_MAX_BUFFER_SIZE - 1;
	else if (rsize > HID_MAX_BUFFER_SIZE)
		rsize = HID_MAX_BUFFER_SIZE;
	..

// drivers/staging/greybus/hid.c
static int gb_hid_start(..)
{
	..
	if (bufsize > HID_MAX_BUFFER_SIZE)
		bufsize = HID_MAX_BUFFER_SIZE;
	..

> How about instead just rejecting any device which includes a report 
> whose length is too big (along with an line in the system log explaining 
> what's wrong)?

Not everywhere, but there are already such as logs when > HID_MAX_BUFFER_SIZE

// drivers/hid/hidraw.c
static ssize_t hidraw_send_report(..)
{
	..
	if (count > HID_MAX_BUFFER_SIZE) {
		hid_warn(dev, "pid %d passed too large report\n",
			 task_pid_nr(current));
		ret = -EINVAL;
		goto out;
	}


I've just noticed that hid_compute_report_size() doing the same thing as
hid_report_len(). So I will replace it with latter one with length check.

So in [PATCH v2] I will do following:

 1. replace hid_compute_report_size() with hid_report_len()

 2. in hid_report_len() we can hid_warn() if length > HID_MAX_BUFFER_SIZE,
and return HID_MAX_BUFFER_SIZE. Or we can return 0 in hid_report_len() to let
functions like hid_hw_raw_request(), hid_hw_output_report() to validate
invalid report length and return -EINVAL. Though I'll need to add !length
missing checks in other places.

Please let me know what it's preferred way in 2nd step.